Skip to content

Conversation

@dpolishuk
Copy link

@dpolishuk dpolishuk commented Oct 18, 2025

Summary

This PR introduces a comprehensive command history persistence system that enables bash commands to be stored in the database and searched intelligently within the chat editor interface. The implementation provides seamless history navigation using arrow keys and maintains a rolling history of commands across sessions.

Key Features

  • Database Integration: New command_history table with automatic migrations
  • CommandHistory Service: Dedicated service for managing command persistence operations
  • Enhanced Chat Editor: Keyboard navigation (↑/↓) for browsing command history
  • Intelligent Search: Quick access to previously executed commands during conversations
  • Session Isolation: Commands are scoped per session with automatic cleanup
  • Rolling History: Maintains up to 1000 commands per session with automatic cleanup

Implementation Details

Database Schema

  • New command_history table with fields for id, session_id, command, created_at, updated_at
  • Migration script ensures seamless rollout to existing installations
  • SQL queries generated via sqlc for type safety and performance

Service Layer

  • CommandHistory service implements pub/sub pattern for real-time updates
  • Automatic history size management (max 1000 entries per session)
  • Session-scoped history with cleanup capabilities

UI Integration

  • Enhanced chat editor with history navigation shortcuts
  • Up arrow (↑): navigate to previous commands
  • Down arrow (↓): navigate to newer commands
  • Preserves current input while browsing history
  • Seamless integration with existing editor functionality

Testing

  • Comprehensive unit tests for history navigation logic
  • Session management test coverage
  • Integration tests for database operations
  • UI component tests for keyboard interactions

Breaking Changes

None. This is a pure additive feature with backward compatibility maintained.

Test Plan

  • Verify database migrations run correctly
  • Test command persistence after bash execution
  • Test history navigation in chat editor (↑/↓ keys)
  • Verify session isolation of command history
  • Test history size limits and cleanup
  • Verify no impact on existing functionality

💘 Generated with Crush

This commit introduces a comprehensive command history persistence system that
stores bash commands in the database and enables intelligent history search
functionality within the chat editor.

Key components:
- Database schema for command history with automatic migrations
- CommandHistory service for persistence operations
- Enhanced chat editor with history navigation (Ctrl+R/Ctrl+P/Ctrl+N)
- Full-text search capabilities for command discovery
- Background command tracking integration

The system maintains a rolling history of commands and provides seamless
keyboard navigation for quick command retrieval during conversations.

💘 Generated with Crush
Co-Authored-By: Crush <[email protected]>
@dpolishuk dpolishuk requested a review from a team as a code owner October 18, 2025 11:04
@dpolishuk dpolishuk requested review from Copilot, meowgorithm and tauraamui and removed request for a team and Copilot October 18, 2025 11:04
@charmcli
Copy link
Contributor

charmcli commented Oct 18, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

Copilot AI review requested due to automatic review settings October 18, 2025 11:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds persistent per-session command history to the app with DB-backed storage and TUI navigation (↑/↓). Key changes include new DB schema and queries, a command history service with size limits, and TUI integration to add, load, and navigate history.

  • Introduces command_history table, queries, and migration.
  • Adds commandhistory.Service and integrates it into App.
  • Implements TUI editor history navigation and storage on send; adds tests.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
internal/tui/components/chat/editor/keys.go Adds ↑/↓ bindings to navigate history.
internal/tui/components/chat/editor/history_test.go Adds unit tests for navigating, adding history, and session resets.
internal/tui/components/chat/editor/editor.go Loads/saves history, handles ↑/↓ navigation, and integrates history on send and session changes.
internal/db/sql/command_history.sql Adds SQL queries for create, list (by session and latest), delete, and count.
internal/db/querier.go Extends Querier with command history methods.
internal/db/models.go Adds models and params for command history.
internal/db/migrations/20251018000000_add_command_history.sql Adds schema, indexes, and trigger for updated_at.
internal/db/db.go Prepares/closes statements for new queries.
internal/db/command_history.sql.go Generated query implementations for command history.
internal/commandhistory/service.go Implements command history service with size cap and pubsub.
internal/app/app.go Wires CommandHistory service into App.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +446 to +468
}

newIndex := m.historyIndex + direction

if direction < 0 { // Up arrow
if newIndex >= 0 {
m.historyIndex = newIndex
// Since history is in descending order (latest first),
// we can access directly by index
m.textarea.SetValue(m.history[m.historyIndex].Command)
m.textarea.CursorEnd()
}
} else { // Down arrow
if newIndex < len(m.history) {
m.historyIndex = newIndex
m.textarea.SetValue(m.history[m.historyIndex].Command)
m.textarea.CursorEnd()
} else {
// Return to saved input
m.textarea.SetValue(m.tempInput)
m.textarea.CursorEnd()
m.isInHistoryMode = false
m.historyIndex = len(m.history)
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

With history sorted in descending order (latest first), initializing historyIndex to len(m.history) makes the first Up (direction = -1) select the last element (oldest), not the most recent. To show the latest entry on the first Up, set the index on entry to 0 and adjust the increment/decrement logic accordingly.

Suggested change
}
newIndex := m.historyIndex + direction
if direction < 0 { // Up arrow
if newIndex >= 0 {
m.historyIndex = newIndex
// Since history is in descending order (latest first),
// we can access directly by index
m.textarea.SetValue(m.history[m.historyIndex].Command)
m.textarea.CursorEnd()
}
} else { // Down arrow
if newIndex < len(m.history) {
m.historyIndex = newIndex
m.textarea.SetValue(m.history[m.historyIndex].Command)
m.textarea.CursorEnd()
} else {
// Return to saved input
m.textarea.SetValue(m.tempInput)
m.textarea.CursorEnd()
m.isInHistoryMode = false
m.historyIndex = len(m.history)
m.historyIndex = -1 // Start before the first history entry
}
// Up arrow: direction == -1, Down arrow: direction == +1
if direction < 0 { // Up arrow
if m.historyIndex < len(m.history)-1 {
m.historyIndex++
m.textarea.SetValue(m.history[m.historyIndex].Command)
m.textarea.CursorEnd()
}
} else { // Down arrow
if m.historyIndex > 0 {
m.historyIndex--
m.textarea.SetValue(m.history[m.historyIndex].Command)
m.textarea.CursorEnd()
} else if m.historyIndex == 0 {
// Return to saved input
m.textarea.SetValue(m.tempInput)
m.textarea.CursorEnd()
m.isInHistoryMode = false
m.historyIndex = -1

Copilot uses AI. Check for mistakes.
return nil
}

history, err := m.app.CommandHistory.ListBySession(ctx, m.session.ID, 0) // 0 = no limit
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

The comment '0 = no limit' conflicts with Service.ListBySession, which treats limit <= 0 as MaxHistorySize (1000). Either change the service to interpret 0 as truly 'no limit' (e.g., call the unbounded listing query) or update this comment to reflect the actual behavior.

Suggested change
history, err := m.app.CommandHistory.ListBySession(ctx, m.session.ID, 0) // 0 = no limit
history, err := m.app.CommandHistory.ListBySession(ctx, m.session.ID, 0) // 0 = use default max history size (1000)

Copilot uses AI. Check for mistakes.
for i := 0; i < toRemove && i < len(history); i++ {
// Simple deletion - in a more sophisticated implementation,
// we might want to batch delete
s.db.ExecContext(ctx, "DELETE FROM command_history WHERE id = ?", history[i].ID)
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

The delete error is ignored; if DELETE fails, Add will silently proceed. Capture and return the error to avoid data inconsistency.

Suggested change
s.db.ExecContext(ctx, "DELETE FROM command_history WHERE id = ?", history[i].ID)
_, err := s.db.ExecContext(ctx, "DELETE FROM command_history WHERE id = ?", history[i].ID)
if err != nil {
return CommandHistory{}, err
}

Copilot uses AI. Check for mistakes.
Comment on lines 67 to 73
// Remove oldest entries to make room
toRemove := int(countRow.Count) - MaxHistorySize + 1
for i := 0; i < toRemove && i < len(history); i++ {
// Simple deletion - in a more sophisticated implementation,
// we might want to batch delete
s.db.ExecContext(ctx, "DELETE FROM command_history WHERE id = ?", history[i].ID)
}
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

This performs N round-trips to delete one-by-one. Use a single statement to delete the oldest N rows atomically, e.g., DELETE with a subselect over ordered ids (and ideally wrap the count, delete, and insert in a transaction).

Copilot uses AI. Check for mistakes.
SELECT *
FROM command_history
WHERE session_id = ?
ORDER BY created_at DESC;
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

This query orders DESC, but the generated code for ListCommandHistoryBySession orders ASC. This mismatch will flip behavior on the next sqlc regeneration. Align the SQL to ASC (or regenerate the code after updating the SQL) to ensure the source and generated code match.

Suggested change
ORDER BY created_at DESC;
ORDER BY created_at ASC;

Copilot uses AI. Check for mistakes.
CREATE TRIGGER IF NOT EXISTS update_command_history_updated_at
AFTER UPDATE ON command_history
BEGIN
UPDATE command_history SET updated_at = strftime('%s', 'now')
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Timestamps are documented as milliseconds, but values are set with strftime('%s','now'), which yields seconds. Either update the comments or multiply by 1000 to store milliseconds consistently.

Suggested change
UPDATE command_history SET updated_at = strftime('%s', 'now')
UPDATE command_history SET updated_at = (strftime('%s', 'now') * 1000)

Copilot uses AI. Check for mistakes.
AFTER UPDATE ON command_history
BEGIN
UPDATE command_history SET updated_at = strftime('%s', 'now')
WHERE id = new.id;
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Updating the same table in an AFTER UPDATE trigger can recurse if recursive_triggers is enabled. Add a guard to prevent re-updating the row (e.g., WHERE id = NEW.id AND updated_at = NEW.updated_at) to ensure the second invocation does no-op and recursion terminates.

Suggested change
WHERE id = new.id;
WHERE id = new.id AND updated_at = new.updated_at;

Copilot uses AI. Check for mistakes.
Comment on lines +98 to +105
if limit <= 0 {
limit = MaxHistorySize
}

dbHistory, err := s.q.ListLatestCommandHistoryBySession(ctx, db.ListLatestCommandHistoryBySessionParams{
SessionID: sessionID,
Limit: int64(limit),
})
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

The method signature suggests 'limit' is caller-controlled, but treating limit <= 0 as MaxHistorySize contradicts the caller comment that 0 = no limit. Consider: if limit <= 0, call the unbounded list (ListCommandHistoryBySession) or document that 0 means 'use default cap'.

Suggested change
if limit <= 0 {
limit = MaxHistorySize
}
dbHistory, err := s.q.ListLatestCommandHistoryBySession(ctx, db.ListLatestCommandHistoryBySessionParams{
SessionID: sessionID,
Limit: int64(limit),
})
var dbHistory []db.CommandHistory
var err error
if limit <= 0 {
// Unbounded: get all history for the session
dbHistory, err = s.q.ListCommandHistoryBySession(ctx, sessionID)
} else {
dbHistory, err = s.q.ListLatestCommandHistoryBySession(ctx, db.ListLatestCommandHistoryBySessionParams{
SessionID: sessionID,
Limit: int64(limit),
})
}

Copilot uses AI. Check for mistakes.
return nil
}

history, err := m.app.CommandHistory.ListBySession(ctx, m.session.ID, 0) // 0 = no limit
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Given ListBySession returns history in descending order (latest first), setting historyIndex to len(m.history) causes the first Up to select the oldest. Either reverse the slice locally or initialize the index/logic so the first Up displays the latest entry.

Copilot uses AI. Check for mistakes.
Comment on lines +431 to +432
m.history = history
m.historyIndex = len(m.history)
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Given ListBySession returns history in descending order (latest first), setting historyIndex to len(m.history) causes the first Up to select the oldest. Either reverse the slice locally or initialize the index/logic so the first Up displays the latest entry.

Copilot uses AI. Check for mistakes.
@dpolishuk dpolishuk force-pushed the feat/command-history-single branch from ca4252c to 8853d25 Compare October 18, 2025 11:55
@dpolishuk
Copy link
Author

I have read the Contributor License Agreement (CLA) and hereby sign the CLA.

@dpolishuk dpolishuk requested a review from Copilot October 18, 2025 20:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +7 to +8
created_at INTEGER NOT NULL, -- Unix timestamp in milliseconds
updated_at INTEGER NOT NULL, -- Unix timestamp in milliseconds
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

The comments state milliseconds, but values are set via strftime('%s','now'), which returns seconds. Update the comments to 'seconds' or change the insert/trigger to store milliseconds (e.g., strftime('%s','now')*1000) to match the documented units.

Copilot uses AI. Check for mistakes.
if direction < 0 { // Up arrow
if newIndex >= 0 {
m.historyIndex = newIndex
// Since history is in descending order (latest first),
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

This comment is inaccurate: ListBySession returns history in chronological order (oldest first). The indexing logic still works because historyIndex starts at len(history) and moves backward, but the comment should reflect chronological order to avoid confusion.

Suggested change
// Since history is in descending order (latest first),
// Since history is in chronological order (oldest first),

Copilot uses AI. Check for mistakes.
Comment on lines +60 to +75
history, err := s.q.ListCommandHistoryBySession(ctx, db.ListCommandHistoryBySessionParams{
SessionID: sessionID,
})
if err != nil {
return CommandHistory{}, err
}

// Remove oldest entries to make room
toRemove := int(countRow.Count) - MaxHistorySize + 1
for i := 0; i < toRemove && i < len(history); i++ {
// Simple deletion - in a more sophisticated implementation,
// we might want to batch delete
if _, err := s.db.ExecContext(ctx, "DELETE FROM command_history WHERE id = ?", history[i].ID); err != nil {
return CommandHistory{}, err
}
}
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Deleting oldest entries one-by-one after loading the full history is inefficient. Prefer a single statement that deletes the oldest N rows, e.g., DELETE by id IN a subquery limited by created_at ASC, to avoid loading and N round-trips.

Suggested change
history, err := s.q.ListCommandHistoryBySession(ctx, db.ListCommandHistoryBySessionParams{
SessionID: sessionID,
})
if err != nil {
return CommandHistory{}, err
}
// Remove oldest entries to make room
toRemove := int(countRow.Count) - MaxHistorySize + 1
for i := 0; i < toRemove && i < len(history); i++ {
// Simple deletion - in a more sophisticated implementation,
// we might want to batch delete
if _, err := s.db.ExecContext(ctx, "DELETE FROM command_history WHERE id = ?", history[i].ID); err != nil {
return CommandHistory{}, err
}
}
toRemove := int(countRow.Count) - MaxHistorySize + 1
// Batch delete oldest entries in a single statement
_, err := s.db.ExecContext(ctx, `
DELETE FROM command_history
WHERE id IN (
SELECT id FROM command_history
WHERE session_id = ?
ORDER BY created_at ASC
LIMIT ?
)
`, sessionID, toRemove)
if err != nil {
return CommandHistory{}, err
}

Copilot uses AI. Check for mistakes.
Comment on lines +318 to +322
if key.Matches(msg, m.keyMap.HistoryUp) {
return m, m.navigateHistory(-1)
}
if key.Matches(msg, m.keyMap.HistoryDown) {
return m, m.navigateHistory(1)
Copy link

Copilot AI Oct 18, 2025

Choose a reason for hiding this comment

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

Intercepting Up/Down unconditionally prevents normal cursor navigation in multi-line inputs. Gate history navigation to when the caret is at the first line for Up and at the last line for Down (or when there is a single line), so standard textarea navigation remains usable.

Suggested change
if key.Matches(msg, m.keyMap.HistoryUp) {
return m, m.navigateHistory(-1)
}
if key.Matches(msg, m.keyMap.HistoryDown) {
return m, m.navigateHistory(1)
caret := m.textarea.Cursor()
value := m.textarea.Value()
lines := strings.Split(value, "\n")
// Find caret line number
caretLine := 0
caretPos := caret
pos := 0
for i, line := range lines {
if caretPos <= pos+len(line) {
caretLine = i
break
}
pos += len(line) + 1 // +1 for '\n'
}
if key.Matches(msg, m.keyMap.HistoryUp) {
if caretLine == 0 || len(lines) == 1 {
return m, m.navigateHistory(-1)
}
}
if key.Matches(msg, m.keyMap.HistoryDown) {
if caretLine == len(lines)-1 || len(lines) == 1 {
return m, m.navigateHistory(1)
}

Copilot uses AI. Check for mistakes.
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