mirror of
https://github.com/getcompanion-ai/co-mono.git
synced 2026-04-15 06:04:40 +00:00
Fix tree selector focus behavior (#1142)
* 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)
This commit is contained in:
parent
43be54c237
commit
4ca7bbe450
3 changed files with 332 additions and 29 deletions
|
|
@ -59,6 +59,7 @@ class TreeList implements Component {
|
|||
private toolCallMap: Map<string, ToolCallInfo> = new Map();
|
||||
private multipleRoots = false;
|
||||
private activePathIds: Set<string> = 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<string, FlatNode>();
|
||||
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<string, number>(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;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
283
packages/coding-agent/test/tree-selector.test.ts
Normal file
283
packages/coding-agent/test/tree-selector.test.ts
Normal file
|
|
@ -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<SessionEntry>): SessionTreeNode[] {
|
||||
if (entries.length === 0) return [];
|
||||
|
||||
const nodes: SessionTreeNode[] = entries.map((entry) => ({
|
||||
entry,
|
||||
children: [],
|
||||
}));
|
||||
|
||||
const byId = new Map<string, SessionTreeNode>();
|
||||
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");
|
||||
});
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Add a link
Reference in a new issue