Skip to content

feat(tui): add focused transcript actions - #5652

Merged
Hmbown merged 3 commits into
mainfrom
codex/pr5608-rescue-20260827
Aug 27, 2026
Merged

feat(tui): add focused transcript actions#5652
Hmbown merged 3 commits into
mainfrom
codex/pr5608-rescue-20260827

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Summary

Rescue of @wuisabel-gif's #5608 onto current main. Authorship of the feature and paste-burst fix commits is preserved (Isabel Wu <231155141+wuisabel-gif@users.noreply.github.com>). The original PR was closed as already integrated on a 0.9.12 integration branch, but the work never reached origin/main.

When the transcript is focused and the composer is empty, the current focused block now supports:

  • y — copy canonical block content;
  • Y — copy the rendered metadata/receipt view;
  • Enter — open the block in a fullscreen readable pager;
  • r — open the existing raw detail view.

y/Y/r are handled after the paste-burst ambiguity window resolves and require an active transcript selection, so a standing selection cannot steal characters from a typed command such as /plugin trust demo. Tasks rail y/Y shortcuts remain unchanged.

docs/KEYBINDINGS.md and the changelog are updated.

Original: #5608

Testing

  • python3 scripts/check-coauthor-trailers.py --range origin/main..HEAD (passed for 3 commits)
  • cargo test -p codewhale-tui --lib 'tui::ui::activity_detail' --locked (11 passed, including focused_pager_and_metadata_copy_use_the_same_cell_target)
  • cargo test -p codewhale-tui --lib typed_command_burst_keeps_r_and_y_out_of_transcript_actions --locked (1 passed)
  • git diff --check

No-Issue: Re-lands the already-reviewed #5608 / #5551 transcript-action slice; does not close the broader #5551 issue.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 3m 55s —— View job


Claude finished reviewing this PR

Todo list

  • Gather context (diff, changed files, related discussion)
  • Understand the request (code review per custom instructions)
  • Review correctness/panics
  • Review provider/model/route safety implications (N/A — no provider/model code touched)
  • Review reuse/simplification
  • Review test coverage
  • Review security implications
  • Post final review

Summary

This rescues #5608's transcript-action shortcuts (y/Y/Enter/r) onto current main, preserving Isabel Wu's authorship. The paste-burst guarding for y/Y/r (only fire when app.input.is_empty() and only after the ambiguity window resolves the char as "typed") is a sound approach for the character-key shortcuts, and the added focused_pager_and_metadata_copy_use_the_same_cell_target / typed_command_burst_keeps_r_and_y_out_of_transcript_actions tests are useful, focused regression coverage. No provider/model/route code is touched, so EPIC #2608 concerns don't apply here.

However, I found two correctness issues worth fixing before merge.

1. Stale transcript selection can eat the first character of a new prompt (crates/tui/src/tui/ui/event_loop.rs:119-130)

fn handle_focused_transcript_action_char(app: &mut App, ch: char) -> bool {
    if !app.input.is_empty() || !app.viewport.transcript_selection.is_active() {
        return false;
    }
    match ch {
        'y' => copy_focused_cell(app),
        'Y' => copy_focused_cell_metadata(app),
        'r' => detail_target_cell_index(app)
            .is_some_and(|index| open_details_pager_for_cell(app, index)),
        _ => false,
    }
}

TranscriptSelection::is_active() (crates/tui/src/tui/selection.rs:46-48) just checks anchor.is_some() && head.is_some(). Once a user makes a mouse-drag selection in the transcript (e.g. to copy an earlier tool output), that selection stays "active" indefinitely — it's only cleared on resize, session load, or a couple of explicit-clear call sites (app.rs:4892, apply.rs:3049, mouse_ui.rs:1655, commands/groups/core/core.rs:199). It is not cleared when the user starts typing a new prompt.

Since handle_focused_transcript_action_char only runs on the single character flushed from the paste-burst ambiguity window — i.e. precisely the first keystroke of a fresh, empty composer — any leftover selection from earlier in the session will silently swallow the next prompt's first character if it happens to be r, y, or Y (e.g. typing "run the tests", "review this", "yes, continue", or /rewind). Instead of typing that letter, it pops open a raw-detail pager or fires a clipboard copy, and the keystroke is lost from the composer.

The added test typed_command_burst_keeps_r_and_y_out_of_transcript_actions (crates/tui/src/tui/ui/tests.rs) sets up an active selection but only types /plugin trust demo, whose first character is /, so it never exercises this path — the r in "trust" arrives after app.input is already non-empty and is correctly untouched. The actual first-character-is-r/y case isn't covered.

Given the PR's stated goal ("a standing selection [must not] steal characters from a typed command"), this is the same failure mode the PR is trying to fix, just not fully closed. A fix likely needs the shortcut to require more than "a selection exists somewhere" — e.g. also require the selection to still be visible/relevant (or clear transcript_selection when composer input begins), similar to how owner.identity_epoch != app.transcript_identity_epoch is used elsewhere (handle_transcript_space, event_loop.rs:68) to detect a stale/rendered-over target.

Fix this →

2. New Enter arm shadows the existing tool-run-expansion Enter shortcut (crates/tui/src/tui/ui/event_loop.rs:4712-4735)

KeyCode::Enter
    if key.modifiers == KeyModifiers::NONE
        && app.input.is_empty()
        && app.viewport.transcript_selection.is_active()
        && open_pager_for_selection(app) => { continue; }
KeyCode::Enter                                    // <-- new arm
    if key.modifiers == KeyModifiers::NONE
        && app.input.is_empty()
        && detail_target_cell_index(app).is_some()
        && open_focused_cell_pager(app) => { continue; }
KeyCode::Enter                                    // <-- now unreachable in practice
    if key.modifiers == KeyModifiers::NONE
        && app.input.is_empty()
        && detail_target_cell_index(app)
            .is_some_and(|idx| app.toggle_tool_run_expansion_at(idx)) => { continue; }

detail_target_cell_index (activity_detail.rs:631-647) falls back to app.virtual_cell_count().checked_sub(1) when there's no selection/viewport target, so it resolves to Some for any non-empty history. open_focused_cell_pager then always returns true once it gets a valid cell index (activity_detail.rs:506-524). That means the new second arm's guard is satisfied — and the arm consumes the Enter keypress — in essentially every case where the composer is empty and history is non-empty, including when the focused/last cell is a collapsed tool-run group. The third arm's toggle_tool_run_expansion_at call is now unreachable via Enter.

Tool-run expansion is still reachable via Space (handle_transcript_space, event_loop.rs:89) and mouse click (mouse_ui.rs:159), so this isn't a total loss of functionality, but it is a silent behavior change/dead-code regression for anyone who used Enter to expand/collapse a tool-run group, and it isn't mentioned in the PR description or docs/KEYBINDINGS.md. Worth either: (a) reordering so the tool-run-toggle arm is tried before the generic pager-open arm, or (b) confirming this is an intentional behavior change and documenting it.

Fix this →

Other notes (not blocking)

  • copy_focused_cell_metadata (activity_detail.rs:550-583) and open_focused_cell_pager (activity_detail.rs:506-524) duplicate the last_transcript_area.map(|a| a.width).unwrap_or(80) pattern already repeated at several other call sites in this file (e.g. copy_focused_cell, open_pager_for_last_message). Not new to this PR, but a small shared helper would reduce the duplication this PR adds to.
  • Authorship/co-author trailers and changelog updates look properly carried over per the PR description's rescue narrative.

wuisabel-gif and others added 3 commits August 27, 2026 06:50
Add content, metadata, fullscreen, and raw-detail actions for the focused transcript cell while preserving Tasks rail y/Y behavior. Document the shortcuts and cover focused pager/copy paths for #5551.
Route y/Y/r only after the paste-burst ambiguity window resolves and require an active transcript selection. Preserve typed command characters and add a regression for fast /plugin commands.
Strengthen the contributed paste-burst regression with an active transcript selection, so it proves a standing selection cannot steal characters from /plugin trust demo while preserving the intended settled-key action.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@Hmbown
Hmbown force-pushed the codex/pr5608-rescue-20260827 branch from 14fb762 to b7b2c55 Compare August 27, 2026 13:50

Hmbown commented Aug 27, 2026

Copy link
Copy Markdown
Owner Author

#5653 (the schema-cost rescue of #5603) is now on main. This branch was rebased onto that merge (d4d9a567d) so the two changelog bullets can land independently. Isabel's feature/fix authorship is unchanged.

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 26s —— View job


Summary

I re-reviewed the diff (origin/main...HEAD, same 3 commits / same file SHAs as the prior review — no new pushes since). This rescues #5608's transcript-action shortcuts (y/Y/Enter/r) onto current main, preserving Isabel Wu's authorship. The paste-burst guarding approach for the character-key shortcuts (only fire when app.input.is_empty() and only after the ambiguity window resolves the char as "typed") is sound, and the two added tests are useful, focused regression coverage. No provider/model/route code is touched, so EPIC #2608 concerns don't apply here.

The two correctness issues from the earlier review are still present in the current code (unchanged since that pass):

1. Stale transcript selection can eat the first character of a new prompt (crates/tui/src/tui/ui/event_loop.rs:119-130)

fn handle_focused_transcript_action_char(app: &mut App, ch: char) -> bool {
    if !app.input.is_empty() || !app.viewport.transcript_selection.is_active() {
        return false;
    }
    match ch {
        'y' => copy_focused_cell(app),
        'Y' => copy_focused_cell_metadata(app),
        'r' => detail_target_cell_index(app)
            .is_some_and(|index| open_details_pager_for_cell(app, index)),
        _ => false,
    }
}

TranscriptSelection::is_active() (crates/tui/src/tui/selection.rs:46-48) just checks anchor.is_some() && head.is_some(). Once a user makes a mouse-drag selection in the transcript, that selection stays "active" indefinitely — it's cleared only at a few explicit sites (app.rs:4892, apply.rs:3049, mouse_ui.rs:1655, commands/groups/core/core.rs:199), none of which fire when the user simply starts typing a new prompt.

Since handle_focused_transcript_action_char runs on the single character flushed from the paste-burst ambiguity window — i.e. precisely the first keystroke of a fresh, empty composer — any leftover selection from earlier in the session will silently swallow the next prompt's first character if it's r, y, or Y (e.g. typing "run the tests", "yes, continue", /rewind). Instead of that letter landing in the composer, it fires a clipboard copy or opens a raw-detail pager.

The added test typed_command_burst_keeps_r_and_y_out_of_transcript_actions (crates/tui/src/tui/ui/tests.rs:4533) sets up an active selection but types /plugin trust demo, whose first character is / — so r in "trust" arrives after app.input is already non-empty and is correctly untouched. The first-character-is-r/y case with a stale selection isn't covered by any test.

This is the same failure mode the PR's stated goal ("a standing selection [must not] steal characters from a typed command") is trying to close — just not fully closed. Compare with handle_transcript_space (event_loop.rs:62-70), which checks owner.identity_epoch != app.transcript_identity_epoch to detect a stale/rendered-over target before acting; handle_focused_transcript_action_char has no equivalent freshness check.

Fix this →

2. New Enter arm shadows the existing tool-run-expansion Enter shortcut (crates/tui/src/tui/ui/event_loop.rs:4712-4735)

KeyCode::Enter
    if key.modifiers == KeyModifiers::NONE
        && app.input.is_empty()
        && app.viewport.transcript_selection.is_active()
        && open_pager_for_selection(app) => { continue; }
KeyCode::Enter                                    // <-- new arm
    if key.modifiers == KeyModifiers::NONE
        && app.input.is_empty()
        && detail_target_cell_index(app).is_some()
        && open_focused_cell_pager(app) => { continue; }
KeyCode::Enter                                    // <-- now unreachable in practice
    if key.modifiers == KeyModifiers::NONE
        && app.input.is_empty()
        && detail_target_cell_index(app)
            .is_some_and(|idx| app.toggle_tool_run_expansion_at(idx)) => { continue; }

detail_target_cell_index (activity_detail.rs:631-647) falls back to app.virtual_cell_count().checked_sub(1) when there's no selection/viewport target, so it resolves to Some for any non-empty history. open_focused_cell_pager then always returns true once it has a valid cell index (activity_detail.rs:506-524 — it only returns false on a None index). That means the new second arm's guard is satisfied, and it consumes the Enter keypress, in essentially every case where the composer is empty and history is non-empty — including when the focused/last cell is a collapsed tool-run group. The third arm's toggle_tool_run_expansion_at call is now unreachable via Enter.

Tool-run expansion is still reachable via Space (handle_transcript_space, event_loop.rs:89) and mouse click (mouse_ui.rs:159), so it's not a total loss of functionality, but it's a silent behavior change/dead-code regression for anyone who used Enter to expand/collapse a tool-run group, and it isn't mentioned in the PR description or docs/KEYBINDINGS.md.

Fix this →

Other notes (not blocking)

  • copy_focused_cell_metadata and open_focused_cell_pager (activity_detail.rs) duplicate the last_transcript_area.map(|a| a.width).unwrap_or(80) pattern already repeated at several other call sites in this file (e.g. copy_focused_cell, open_pager_for_last_message). Not new to this PR, but a small shared helper would reduce the duplication this PR adds.
  • Authorship/co-author trailers and changelog updates look properly carried over per the PR description's rescue narrative.

Status

No new commits landed since the earlier automated pass on this PR — the diff, file SHAs, and both findings above are unchanged. Recommend addressing #1 (stale selection eating composer input) and #2 (Enter shortcut shadowing) before merge; neither is a compile-breaking or panic-causing bug, but #1 is a user-visible data-loss-of-a-keystroke bug that directly undermines the PR's own stated goal.

@Hmbown
Hmbown merged commit aed9f5c into main Aug 27, 2026
39 of 40 checks passed
@Hmbown
Hmbown deleted the codex/pr5608-rescue-20260827 branch August 27, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants