Skip to content

feat(tui): add read-only transcript navigation - #110

Open
danielkov wants to merge 1 commit into
mainfrom
feat/transcript-navigation
Open

feat(tui): add read-only transcript navigation#110
danielkov wants to merge 1 commit into
mainfrom
feat/transcript-navigation

Conversation

@danielkov

Copy link
Copy Markdown
Contributor

Summary

Add a read-only transcript navigator, opened with F3 or /transcript, with text search, role filters, previews, and user-prompt jumps. Reveal the selected block without changing conversation history or the parked prompt.

Impact

Navigation remains available while streaming and covers currently displayed or replayed history, not compacted archives. Paste and modal input remain isolated from the composer; search queries are limited to 4,096 UTF-8 bytes.

Technical details

Stable selection within the active session

Selection uses ephemeral block identities separate from tool focus. Wrapped render offsets reveal the chosen block and retain its anchor through terminal resize; these display identities are not durable continuation addresses.

@kit-code-agent kit-code-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three issues remain: viewport changes can restore a stale reveal anchor after automatic following resumes; slow navigator processing can discard deliberate Enter/Tab input; and oversized Unicode graphemes defeat the preview work and allocation bounds.

Comment thread src/tui/app.rs

/// Resolve only after the wrapping cache has current-width prefix offsets.
pub(super) fn apply_navigation_reveal(&mut self, viewport_changed: bool) {
if !(self.navigation.reveal_pending || viewport_changed && self.navigation.anchored) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Discard the reveal anchor when automatic following resumes

After a historical message is revealed, navigation.anchored remains true. A subsequent Update::State(Running | RequiresAction) restores follow = true and bottom scrolling without clearing that anchor. When the terminal width or transcript viewport height next changes, this guard accepts the stale anchor and the code below sets follow = false, jumping back to the historical message instead of following the current response. Clear the anchor when automatic following resumes, or suppress resize-only reanchoring while following, while preserving explicit reveal_pending requests. Cover reveal → Running update → viewport change and assert that following remains enabled.

For agents:
Validate the following issue, address if needed:

<comment>A viewport change reactivates a historical reveal anchor after automatic following resumes, jumping backward and disabling follow; invalidate that anchor or guard resize-only reanchoring while preserving explicit reveals.</comment>
<file_context>
--- a/src/tui/app.rs
+++ b/src/tui/app.rs
@@ -3120,0 +3261,15 @@
+    pub(super) fn apply_navigation_reveal(&mut self, viewport_changed: bool) {
+        if !(self.navigation.reveal_pending || viewport_changed && self.navigation.anchored) {
+            return;
+        }
+        if let Some(index) = self
+            .navigation
+            .revealed
+            .and_then(|id| self.navigation.index(id))
+            && let Some(prefix) = self.transcript_prefixes.get(index)
+        {
+            self.follow = false;
+            self.scroll = prefix + usize::from(*prefix > 0);
+        }
+        self.navigation.reveal_pending = false;
+    }
</file_context>

Comment thread src/tui/app.rs
let result = work(self);
// Do not overwrite a clock changed by the work.
if last_key.is_some() && self.last_key == last_key {
self.last_key = last_key.and_then(|last| last.checked_add(started.elapsed()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Preserve input gaps that occur during slow rendering

Advancing last_key by the entire processing duration also removes real time during which the user can pause and press another key. For example, after a query key at 0 ms, a draw spanning 1–101 ms moves last_key to approximately 100 ms. An intentional Enter received at 80 ms and handled immediately after that draw then appears less than PASTE_GAP (8 ms) after the previous key. handle_navigation_key treats it as pasted whitespace and discards it; Tab is discarded in the same way. Capture event-receipt timestamps independently of synchronous rendering/update processing and classify bursts from those timestamps rather than subtracting all processing time. Add coverage for a deliberate key arriving during slow work, not only a deliberate gap established before that work starts.

For agents:
Validate the following issue, address if needed:

<comment>Subtracting all synchronous processing time from the inter-key gap misclassifies deliberate Enter/Tab events received during slow work as paste; use independently captured event-receipt timing.</comment>
<file_context>
--- a/src/tui/app.rs
+++ b/src/tui/app.rs
@@ -3120,0 +3283,7 @@
+        let last_key = self.last_key.filter(|_| self.navigation.dialog.is_some());
+        let started = Instant::now();
+        let result = work(self);
+        // Do not overwrite a clock changed by the work.
+        if last_key.is_some() && self.last_key == last_key {
+            self.last_key = last_key.and_then(|last| last.checked_add(started.elapsed()));
+        }
</file_context>

Comment thread src/tui/transcript.rs
Comment on lines +194 to +198
let mut words =
text_parts(block).flat_map(|text| text.graphemes(true).chain(std::iter::once(" ")));
let mut preview: String = words
.by_ref()
.take(96)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Bound preview input before grapheme segmentation

The 96-grapheme limit does not bound the bytes processed or retained. Assistant text or tool output containing e followed by a large sequence of U+0301 combining marks forms one grapheme: segmentation scans the entire cluster, the control-character check scans it again, and collection retains its full payload. Whitespace normalization then processes that payload again. Since each visible navigator row calls preview on every redraw, this defeats the intended compact-preview work bound. Even an oversized 97th grapheme is scanned by words.next() without being displayed. Apply a source-byte or character budget before segmentation, safely omit an incomplete boundary cluster, and bound truncation lookahead as well. Cover oversized clusters both within the displayed prefix and immediately after its 96-grapheme limit.

For agents:
Validate the following issue, address if needed:

<comment>A grapheme-count limit permits payload-sized Unicode clusters to be scanned and allocated on every preview redraw; bound source consumption before segmentation and bound lookahead.</comment>
<file_context>
--- /dev/null
+++ b/src/tui/transcript.rs
@@ -0,0 +192,20 @@
+pub(super) fn preview(block: &Block) -> String {
+    // Bound the work even for large tool output and preserve Unicode graphemes.
+    let mut words =
+        text_parts(block).flat_map(|text| text.graphemes(true).chain(std::iter::once(" ")));
+    let mut preview: String = words
+        .by_ref()
+        .take(96)
+        .map(|character| {
+            if character.chars().any(char::is_control) {
+                " "
+            } else {
+                character
+            }
+        })
+        .collect();
+    if words.next().is_some() {
+        preview.push('…');
+    }
+    preview.split_whitespace().collect::<Vec<_>>().join(" ")
+}
</file_context>

Comment thread src/tui/transcript.rs
Comment on lines +226 to +231
.filter(|character| {
#[cfg(test)]
QUERY_INPUT_CHARACTERS.with(|count| count.set(count.get() + 1));
consumed += character.len_utf8();
!character.is_control()
})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't mix tests with implementation, if you need to inject test-specific logic to test it, you're asserting on implementation and not behavior

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.

1 participant