From 4ca7bbe45049f51dcd8d2d8b65f83c4b282af9c9 Mon Sep 17 00:00:00 2001 From: Sviatoslav Abakumov Date: Sun, 1 Feb 2026 16:23:52 +0400 Subject: [PATCH] Fix tree selector focus behavior (#1142) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(coding-agent): tree selector focuses nearest visible ancestor When the selected entry is not visible (filtered out by mode change or a metadata entry like model_change), walk up the parent chain to find the nearest visible ancestor instead of jumping to the last item. Fixes selection behavior for: - Initial selection when currentLeafId is a metadata entry - Filter switching, e.g. Ctrl+U for user-only mode * fix(coding-agent): tree selector preserves selection through empty filters When switching to a filter with no results, e.g. labeled-only with no labels and back, the cursor would reset to the first message instead of the original selection. Track lastSelectedId as a class member and only update it when filteredNodes is non-empty, preserving the selection across empty filter results. * test(coding-agent): add tree selector filter and selection tests - Test metadata entry handling (model_change, thinking_level_change) - Test filter switching with parent traversal (default ↔ user-only) - Test empty filter preservation (labeled-only with no labels) --- .../interactive/components/tree-selector.ts | 63 ++-- .../src/modes/interactive/interactive-mode.ts | 15 +- .../coding-agent/test/tree-selector.test.ts | 283 ++++++++++++++++++ 3 files changed, 332 insertions(+), 29 deletions(-) create mode 100644 packages/coding-agent/test/tree-selector.test.ts diff --git a/packages/coding-agent/src/modes/interactive/components/tree-selector.ts b/packages/coding-agent/src/modes/interactive/components/tree-selector.ts index ab527734..4f049239 100644 --- a/packages/coding-agent/src/modes/interactive/components/tree-selector.ts +++ b/packages/coding-agent/src/modes/interactive/components/tree-selector.ts @@ -59,6 +59,7 @@ class TreeList implements Component { private toolCallMap: Map = new Map(); private multipleRoots = false; private activePathIds: Set = new Set(); + private lastSelectedId: string | null = null; public onSelect?: (entryId: string) => void; public onCancel?: () => void; @@ -79,12 +80,38 @@ class TreeList implements Component { // Start with initialSelectedId if provided, otherwise current leaf const targetId = initialSelectedId ?? currentLeafId; - const targetIndex = this.filteredNodes.findIndex((n) => n.node.entry.id === targetId); - if (targetIndex !== -1) { - this.selectedIndex = targetIndex; - } else { - this.selectedIndex = Math.max(0, this.filteredNodes.length - 1); + this.selectedIndex = this.findNearestVisibleIndex(targetId); + this.lastSelectedId = this.filteredNodes[this.selectedIndex]?.node.entry.id ?? null; + } + + /** + * Find the index of the nearest visible entry, walking up the parent chain if needed. + * Returns the index in filteredNodes, or the last index as fallback. + */ + private findNearestVisibleIndex(entryId: string | null): number { + if (this.filteredNodes.length === 0) return 0; + + // Build a map for parent lookup + const entryMap = new Map(); + for (const flatNode of this.flatNodes) { + entryMap.set(flatNode.node.entry.id, flatNode); } + + // Build a map of visible entry IDs to their indices in filteredNodes + const visibleIdToIndex = new Map(this.filteredNodes.map((node, i) => [node.node.entry.id, i])); + + // Walk from entryId up to root, looking for a visible entry + let currentId = entryId; + while (currentId !== null) { + const index = visibleIdToIndex.get(currentId); + if (index !== undefined) return index; + const node = entryMap.get(currentId); + if (!node) break; + currentId = node.node.entry.parentId ?? null; + } + + // Fallback: last visible entry + return this.filteredNodes.length - 1; } /** Build the set of entry IDs on the path from root to current leaf */ @@ -239,8 +266,11 @@ class TreeList implements Component { } private applyFilter(): void { - // Remember currently selected node to preserve cursor position - const previouslySelectedId = this.filteredNodes[this.selectedIndex]?.node.entry.id; + // Update lastSelectedId only when we have a valid selection (non-empty list) + // This preserves the selection when switching through empty filter results + if (this.filteredNodes.length > 0) { + this.lastSelectedId = this.filteredNodes[this.selectedIndex]?.node.entry.id ?? this.lastSelectedId; + } const searchTokens = this.searchQuery.toLowerCase().split(/\s+/).filter(Boolean); @@ -306,18 +336,17 @@ class TreeList implements Component { // Recalculate visual structure (indent, connectors, gutters) based on visible tree this.recalculateVisualStructure(); - // Try to preserve cursor on the same node after filtering - if (previouslySelectedId) { - const newIndex = this.filteredNodes.findIndex((n) => n.node.entry.id === previouslySelectedId); - if (newIndex !== -1) { - this.selectedIndex = newIndex; - return; - } + // Try to preserve cursor on the same node, or find nearest visible ancestor + if (this.lastSelectedId) { + this.selectedIndex = this.findNearestVisibleIndex(this.lastSelectedId); + } else if (this.selectedIndex >= this.filteredNodes.length) { + // Clamp index if out of bounds + this.selectedIndex = Math.max(0, this.filteredNodes.length - 1); } - // Fall back: clamp index if out of bounds - if (this.selectedIndex >= this.filteredNodes.length) { - this.selectedIndex = Math.max(0, this.filteredNodes.length - 1); + // Update lastSelectedId to the actual selection (may have changed due to parent walk) + if (this.filteredNodes.length > 0) { + this.lastSelectedId = this.filteredNodes[this.selectedIndex]?.node.entry.id ?? this.lastSelectedId; } } diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index e2e6eccf..57b4adf9 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -3352,15 +3352,6 @@ export class InteractiveMode { const tree = this.sessionManager.getTree(); const realLeafId = this.sessionManager.getLeafId(); - // Find the visible leaf for display (skip metadata entries like labels) - let visibleLeafId = realLeafId; - while (visibleLeafId) { - const entry = this.sessionManager.getEntry(visibleLeafId); - if (!entry) break; - if (entry.type !== "label" && entry.type !== "custom") break; - visibleLeafId = entry.parentId ?? null; - } - if (tree.length === 0) { this.showStatus("No entries in session"); return; @@ -3369,11 +3360,11 @@ export class InteractiveMode { this.showSelector((done) => { const selector = new TreeSelectorComponent( tree, - visibleLeafId, + realLeafId, this.ui.terminal.rows, async (entryId) => { - // Selecting the visible leaf is a no-op (already there) - if (entryId === visibleLeafId) { + // Selecting the current leaf is a no-op (already there) + if (entryId === realLeafId) { done(); this.showStatus("Already at this point"); return; diff --git a/packages/coding-agent/test/tree-selector.test.ts b/packages/coding-agent/test/tree-selector.test.ts new file mode 100644 index 00000000..4e22afd1 --- /dev/null +++ b/packages/coding-agent/test/tree-selector.test.ts @@ -0,0 +1,283 @@ +import { beforeAll, describe, expect, test } from "vitest"; +import type { + ModelChangeEntry, + SessionEntry, + SessionMessageEntry, + SessionTreeNode, +} from "../src/core/session-manager.js"; +import { TreeSelectorComponent } from "../src/modes/interactive/components/tree-selector.js"; +import { initTheme } from "../src/modes/interactive/theme/theme.js"; + +beforeAll(() => { + initTheme("dark"); +}); + +// Helper to create a user message entry +function userMessage(id: string, parentId: string | null, content: string): SessionMessageEntry { + return { + type: "message", + id, + parentId, + timestamp: new Date().toISOString(), + message: { role: "user", content, timestamp: Date.now() }, + }; +} + +// Helper to create an assistant message entry +function assistantMessage(id: string, parentId: string | null, text: string): SessionMessageEntry { + return { + type: "message", + id, + parentId, + timestamp: new Date().toISOString(), + message: { + role: "assistant", + content: [{ type: "text", text }], + api: "anthropic-messages", + provider: "anthropic", + model: "claude-sonnet-4", + usage: { + input: 0, + output: 0, + cacheRead: 0, + cacheWrite: 0, + totalTokens: 0, + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, total: 0 }, + }, + stopReason: "stop", + timestamp: Date.now(), + }, + }; +} + +// Helper to create a model_change entry +function modelChange(id: string, parentId: string | null): ModelChangeEntry { + return { + type: "model_change", + id, + parentId, + timestamp: new Date().toISOString(), + provider: "anthropic", + modelId: "claude-sonnet-4", + }; +} + +// Helper to build a tree from entries using parentId relationships +function buildTree(entries: Array): SessionTreeNode[] { + if (entries.length === 0) return []; + + const nodes: SessionTreeNode[] = entries.map((entry) => ({ + entry, + children: [], + })); + + const byId = new Map(); + for (const node of nodes) { + byId.set(node.entry.id, node); + } + + const roots: SessionTreeNode[] = []; + for (const node of nodes) { + if (node.entry.parentId === null) { + roots.push(node); + } else { + const parent = byId.get(node.entry.parentId); + if (parent) { + parent.children.push(node); + } + } + } + return roots; +} + +describe("TreeSelectorComponent", () => { + describe("initial selection with metadata entries", () => { + test("focuses nearest visible ancestor when currentLeafId is a model_change with sibling branch", () => { + // Tree structure: + // user-1 + // └── asst-1 + // ├── user-2 (active branch) + // │ └── model-1 (model_change, CURRENT LEAF) + // └── user-3 (sibling branch, added later chronologically) + const entries = [ + userMessage("user-1", null, "hello"), + assistantMessage("asst-1", "user-1", "hi"), + userMessage("user-2", "asst-1", "active branch"), // Active branch + modelChange("model-1", "user-2"), // Current leaf (metadata) + userMessage("user-3", "asst-1", "sibling branch"), // Sibling branch + ]; + const tree = buildTree(entries); + + const selector = new TreeSelectorComponent( + tree, + "model-1", // currentLeafId is the model_change entry + 24, + () => {}, + () => {}, + ); + + const list = selector.getTreeList(); + // Should focus on user-2 (parent of model-1), not user-3 (last item) + expect(list.getSelectedNode()?.entry.id).toBe("user-2"); + }); + + test("focuses nearest visible ancestor when currentLeafId is a thinking_level_change entry", () => { + // Similar structure with thinking_level_change instead of model_change + const entries = [ + userMessage("user-1", null, "hello"), + assistantMessage("asst-1", "user-1", "hi"), + userMessage("user-2", "asst-1", "active branch"), + { + type: "thinking_level_change" as const, + id: "thinking-1", + parentId: "user-2", + timestamp: new Date().toISOString(), + thinkingLevel: "high", + }, + userMessage("user-3", "asst-1", "sibling branch"), + ]; + const tree = buildTree(entries); + + const selector = new TreeSelectorComponent( + tree, + "thinking-1", + 24, + () => {}, + () => {}, + ); + + const list = selector.getTreeList(); + expect(list.getSelectedNode()?.entry.id).toBe("user-2"); + }); + }); + + describe("filter switching with parent traversal", () => { + test("switches to nearest visible user message when changing to user-only filter", () => { + // In user-only filter: [user-1, user-2, user-3] + const entries = [ + userMessage("user-1", null, "hello"), + assistantMessage("asst-1", "user-1", "hi"), + userMessage("user-2", "asst-1", "active branch"), + assistantMessage("asst-2", "user-2", "response"), + userMessage("user-3", "asst-1", "sibling branch"), + ]; + const tree = buildTree(entries); + + const selector = new TreeSelectorComponent( + tree, + "asst-2", + 24, + () => {}, + () => {}, + ); + + const list = selector.getTreeList(); + expect(list.getSelectedNode()?.entry.id).toBe("asst-2"); + + // Simulate Ctrl+U (user-only filter) + selector.handleInput("\x15"); + + // Should now be on user-2 (the parent user message), not user-3 + expect(list.getSelectedNode()?.entry.id).toBe("user-2"); + }); + + test("returns to nearest visible ancestor when switching back to default filter", () => { + // Same branching structure + const entries = [ + userMessage("user-1", null, "hello"), + assistantMessage("asst-1", "user-1", "hi"), + userMessage("user-2", "asst-1", "active branch"), + assistantMessage("asst-2", "user-2", "response"), + userMessage("user-3", "asst-1", "sibling branch"), + ]; + const tree = buildTree(entries); + + const selector = new TreeSelectorComponent( + tree, + "asst-2", + 24, + () => {}, + () => {}, + ); + + const list = selector.getTreeList(); + expect(list.getSelectedNode()?.entry.id).toBe("asst-2"); + + // Switch to user-only + selector.handleInput("\x15"); // Ctrl+U + expect(list.getSelectedNode()?.entry.id).toBe("user-2"); + + // Switch back to default - should stay on user-2 + // (since that's what we navigated to via parent traversal) + selector.handleInput("\x04"); // Ctrl+D + expect(list.getSelectedNode()?.entry.id).toBe("user-2"); + }); + }); + + describe("empty filter preservation", () => { + test("preserves selection when switching to empty labeled filter and back", () => { + // Tree with no labels + const entries = [ + userMessage("user-1", null, "hello"), + assistantMessage("asst-1", "user-1", "hi"), + userMessage("user-2", "asst-1", "bye"), + assistantMessage("asst-2", "user-2", "goodbye"), + ]; + const tree = buildTree(entries); + + const selector = new TreeSelectorComponent( + tree, + "asst-2", + 24, + () => {}, + () => {}, + ); + + const list = selector.getTreeList(); + expect(list.getSelectedNode()?.entry.id).toBe("asst-2"); + + // Switch to labeled-only filter (no labels exist, so empty result) + selector.handleInput("\x0c"); // Ctrl+L + + // The list should be empty, getSelectedNode returns undefined + expect(list.getSelectedNode()).toBeUndefined(); + + // Switch back to default filter + selector.handleInput("\x04"); // Ctrl+D + + // Should restore to asst-2 (the selection before we switched to empty filter) + expect(list.getSelectedNode()?.entry.id).toBe("asst-2"); + }); + + test("preserves selection through multiple empty filter switches", () => { + const entries = [userMessage("user-1", null, "hello"), assistantMessage("asst-1", "user-1", "hi")]; + const tree = buildTree(entries); + + const selector = new TreeSelectorComponent( + tree, + "asst-1", + 24, + () => {}, + () => {}, + ); + + const list = selector.getTreeList(); + expect(list.getSelectedNode()?.entry.id).toBe("asst-1"); + + // Switch to labeled-only (empty) - Ctrl+L toggles labeled ↔ default + selector.handleInput("\x0c"); // Ctrl+L -> labeled-only + expect(list.getSelectedNode()).toBeUndefined(); + + // Switch to default, then back to labeled-only + selector.handleInput("\x0c"); // Ctrl+L -> default (toggle back) + expect(list.getSelectedNode()?.entry.id).toBe("asst-1"); + + selector.handleInput("\x0c"); // Ctrl+L -> labeled-only again + expect(list.getSelectedNode()).toBeUndefined(); + + // Switch back to default with Ctrl+D + selector.handleInput("\x04"); // Ctrl+D + expect(list.getSelectedNode()?.entry.id).toBe("asst-1"); + }); + }); +});