fix: fix index shifting issue with attachment restoration

Open rstacruz opened this issue 5 months ago • 1 comments

hopefully fixes #1266:

When navigating message history using the "Up" arrow key, messages containing attachments and line breaks become corrupted, resulting in duplicated attachment paths within the text.

Disclaimer: I'm not familiar with the opencode codebase, and this PR was done with the help of opencode. Mostly this is me trying to test out how much OpenCode can help me with a new codebase :)

How I tested this

I used cd packages/tui && go test -v ./... to run tests.

Test output

?   	github.com/sst/opencode/cmd/opencode	[no test files]
?   	github.com/sst/opencode/internal/api	[no test files]
?   	github.com/sst/opencode/internal/app	[no test files]
?   	github.com/sst/opencode/internal/attachment	[no test files]
?   	github.com/sst/opencode/internal/clipboard	[no test files]
?   	github.com/sst/opencode/internal/commands	[no test files]
?   	github.com/sst/opencode/internal/completions	[no test files]
=== RUN   TestRestoreFromPromptWithMultilineAttachments
--- PASS: TestRestoreFromPromptWithMultilineAttachments (0.00s)
=== RUN   TestRestoreFromPromptMultipleAttachments
--- PASS: TestRestoreFromPromptMultipleAttachments (0.00s)
=== RUN   TestRestoreFromPromptAttachmentsAtLineBoundaries
--- PASS: TestRestoreFromPromptAttachmentsAtLineBoundaries (0.00s)
=== RUN   TestRestoreFromPromptEmptyLinesWithAttachments
--- PASS: TestRestoreFromPromptEmptyLinesWithAttachments (0.00s)
=== RUN   TestRestoreFromHistoryIndex
--- PASS: TestRestoreFromHistoryIndex (0.00s)
PASS
ok  	github.com/sst/opencode/internal/components/chat	(cached)
?   	github.com/sst/opencode/internal/components/commands	[no test files]
?   	github.com/sst/opencode/internal/components/dialog	[no test files]
?   	github.com/sst/opencode/internal/components/diff	[no test files]
?   	github.com/sst/opencode/internal/components/fileviewer	[no test files]
=== RUN   TestArrowKeyNavigation
--- PASS: TestArrowKeyNavigation (0.00s)
=== RUN   TestJKKeyNavigation
--- PASS: TestJKKeyNavigation (0.00s)
=== RUN   TestCtrlNavigation
--- PASS: TestCtrlNavigation (0.00s)
=== RUN   TestNavigationBoundaries
--- PASS: TestNavigationBoundaries (0.00s)
=== RUN   TestEmptyList
--- PASS: TestEmptyList (0.00s)
PASS
ok  	github.com/sst/opencode/internal/components/list	(cached)
?   	github.com/sst/opencode/internal/components/modal	[no test files]
?   	github.com/sst/opencode/internal/components/qr	[no test files]
?   	github.com/sst/opencode/internal/components/status	[no test files]
=== RUN   TestReplaceRangeAbsolute
=== RUN   TestReplaceRangeAbsolute/replace_spanning_multiple_lines
=== RUN   TestReplaceRangeAbsolute/replace_across_newlines
=== RUN   TestReplaceRangeAbsolute/replace_spanning_multiple_lines#01
=== RUN   TestReplaceRangeAbsolute/replace_at_start
=== RUN   TestReplaceRangeAbsolute/replace_at_end
=== RUN   TestReplaceRangeAbsolute/empty_replacement
--- PASS: TestReplaceRangeAbsolute (0.00s)
    --- PASS: TestReplaceRangeAbsolute/replace_spanning_multiple_lines (0.00s)
    --- PASS: TestReplaceRangeAbsolute/replace_across_newlines (0.00s)
    --- PASS: TestReplaceRangeAbsolute/replace_spanning_multiple_lines#01 (0.00s)
    --- PASS: TestReplaceRangeAbsolute/replace_at_start (0.00s)
    --- PASS: TestReplaceRangeAbsolute/replace_at_end (0.00s)
    --- PASS: TestReplaceRangeAbsolute/empty_replacement (0.00s)
=== RUN   TestReplaceRangeAbsoluteWithAttachments
--- PASS: TestReplaceRangeAbsoluteWithAttachments (0.00s)
=== RUN   TestAbsolutePosToRowCol
=== RUN   TestAbsolutePosToRowCol/single_line_start
=== RUN   TestAbsolutePosToRowCol/single_line_middle
=== RUN   TestAbsolutePosToRowCol/single_line_end
=== RUN   TestAbsolutePosToRowCol/multi-line_first_line
=== RUN   TestAbsolutePosToRowCol/multi-line_second_line_start
=== RUN   TestAbsolutePosToRowCol/multi-line_second_line_middle
=== RUN   TestAbsolutePosToRowCol/multi-line_third_line
=== RUN   TestAbsolutePosToRowCol/beyond_end
--- PASS: TestAbsolutePosToRowCol (0.00s)
    --- PASS: TestAbsolutePosToRowCol/single_line_start (0.00s)
    --- PASS: TestAbsolutePosToRowCol/single_line_middle (0.00s)
    --- PASS: TestAbsolutePosToRowCol/single_line_end (0.00s)
    --- PASS: TestAbsolutePosToRowCol/multi-line_first_line (0.00s)
    --- PASS: TestAbsolutePosToRowCol/multi-line_second_line_start (0.00s)
    --- PASS: TestAbsolutePosToRowCol/multi-line_second_line_middle (0.00s)
    --- PASS: TestAbsolutePosToRowCol/multi-line_third_line (0.00s)
    --- PASS: TestAbsolutePosToRowCol/beyond_end (0.00s)
=== RUN   TestReplaceRangeAbsoluteEdgeCases
=== RUN   TestReplaceRangeAbsoluteEdgeCases/invalid_positions
=== RUN   TestReplaceRangeAbsoluteEdgeCases/empty_text
=== RUN   TestReplaceRangeAbsoluteEdgeCases/zero-length_replacement
--- PASS: TestReplaceRangeAbsoluteEdgeCases (0.00s)
    --- PASS: TestReplaceRangeAbsoluteEdgeCases/invalid_positions (0.00s)
    --- PASS: TestReplaceRangeAbsoluteEdgeCases/empty_text (0.00s)
    --- PASS: TestReplaceRangeAbsoluteEdgeCases/zero-length_replacement (0.00s)
PASS
ok  	github.com/sst/opencode/internal/components/textarea	(cached)
?   	github.com/sst/opencode/internal/components/toast	[no test files]
?   	github.com/sst/opencode/internal/id	[no test files]
?   	github.com/sst/opencode/internal/layout	[no test files]
?   	github.com/sst/opencode/internal/styles	[no test files]
=== RUN   TestLoadThemesFromJSON
--- PASS: TestLoadThemesFromJSON (0.00s)
=== RUN   TestColorReferenceResolution
--- PASS: TestColorReferenceResolution (0.01s)
=== RUN   TestLoadThemesFromDirectories
--- PASS: TestLoadThemesFromDirectories (0.01s)
PASS
ok  	github.com/sst/opencode/internal/theme	(cached)
?   	github.com/sst/opencode/internal/tui	[no test files]
=== RUN   TestWriteStringsPar
--- PASS: TestWriteStringsPar (0.01s)
PASS
ok  	github.com/sst/opencode/internal/util	(cached)
?   	github.com/sst/opencode/internal/viewport	[no test files]

I used cd packages/opencode && bun run src/index.ts to manually verify.

Left shows the release version (v0.3.57), right shows dev version.

Jul 24 '25 01:07 rstacruz

This below is the implementation plan (generated with the help of OpenCode and spec mode) - this was the prompt that generated this PR.

Full spec...

You are a senior engineer executing a task from a project specification. Implement the tasks in the spec below. Before marking a test as complete, run tests if it makes sense to.

Message history attachment corruption fix

Requirements

Fix attachment path corruption that occurs when navigating message history with the "Up" arrow key for messages containing attachments and line breaks. The issue manifests as duplicated attachment paths being inserted into the text content.

Stories

1. Message history restoration with attachments

Story: AS a user, I WANT to navigate message history containing attachments, SO THAT the restored message displays correctly without corruption.

1.1. WHEN the user presses "Up" arrow to navigate to a previous message with attachments, THEN the system SHALL restore the exact original text and attachment positions
1.2. WHEN the message contains line breaks and attachments, THEN the system SHALL preserve the correct text layout without duplicating attachment paths
1.3. WHEN restoring attachments from history, THEN the system SHALL correctly calculate attachment indices accounting for multi-line text structure

2. Attachment index calculation accuracy

Story: AS the system, I WANT to correctly calculate attachment positions during restoration, SO THAT text replacement operations don't corrupt the message content.

2.1. WHEN processing attachments for restoration, IF the message spans multiple lines, THEN the system SHALL account for newline characters in index calculations
2.2. WHEN replacing text ranges during attachment restoration, THEN the system SHALL use indices that match the original text structure
2.3. WHEN attachments are processed in reverse order, THEN the system SHALL maintain correct relative positions throughout the restoration process

Design

Overview

The bug stems from inconsistent index calculation between message saving and restoration. When saving, GetAttachments() calculates indices including newlines in the position tracking. However, during restoration in RestoreFromHistory, the ReplaceRange operation works on individual text rows without accounting for newlines in the same way.

Root cause analysis

Saving phase: GetAttachments() calculates absolute positions across the entire text including newlines
Restoration phase: RestoreFromHistory uses these absolute indices with ReplaceRange, which operates on the current row
Mismatch: The absolute indices don't align with row-based operations when newlines are present

Files

Modified files

packages/tui/internal/components/chat/editor.go - Update RestoreFromHistory method
packages/tui/internal/components/textarea/textarea.go - Add ReplaceRangeAbsolute method

Reference files for implementation

packages/tui/internal/attachment/attachment.go - Attachment structure reference
packages/tui/internal/app/prompt.go - Prompt structure reference

Component graph

graph TD
    A[RestoreFromHistory] -->|calls| B[textarea.SetValue]
    A -->|calls| C[ReplaceRangeAbsolute]
    A -->|calls| D[InsertAttachment]

    C -->|new method| E[textarea.Model]

    style C fill:#90EE90
    style A fill:#FFFFE0

Components

Updated RestoreFromHistory method

Location: packages/tui/internal/components/chat/editor.go
Replaces current restoration logic with absolute position-aware text replacement
Uses new ReplaceRangeAbsolute method for accurate text manipulation

func (m *editorComponent) RestoreFromHistory(index int) {
    // Validation and setup remain the same
    if index < 0 || index >= len(m.app.State.MessageHistory) {
        return
    }

    entry := m.app.State.MessageHistory[index]
    m.textarea.Reset()
    m.textarea.SetValue(entry.Text)

    // Process attachments using absolute position replacement
    // Sort in reverse order to prevent index shifting
    // Use new absolute replacement method
}

New ReplaceRangeAbsolute method

Location: packages/tui/internal/components/textarea/textarea.go
Handles text replacement using absolute positions across multi-line text
Correctly maps absolute indices to row/column coordinates

func (m *Model) ReplaceRangeAbsolute(startPos, endPos int, replacement string) {
    // Convert absolute positions to row/column coordinates
    // Handle text replacement across potentially multiple rows
    // Maintain cursor positioning consistency
}

Error handling

Maintain existing bounds checking in replacement operations
Add validation for absolute position to row/column conversion
Preserve existing error recovery behavior for malformed history entries

Testing strategy

Run tests with: cd packages/tui && go test -v ./...

// editor_test.go - New test file
func TestRestoreFromHistoryWithAttachments(t *testing.T) {
    // Test multi-line message with attachment
    // Test message with multiple attachments
}

func TestRestoreFromHistoryCorruptionScenario(t *testing.T) {
    // Reproduce exact bug scenario from issue report
    // Verify no duplication occurs
}

// textarea_test.go - Add to existing test file
func TestReplaceRangeAbsolute(t *testing.T) {
    // Test replacement across multiple rows
    // Test replacement with newlines in replacement text
}

Tasks

1. Add absolute position text replacement

[ ] 1.1. Create ReplaceRangeAbsolute method: Add new method to packages/tui/internal/components/textarea/textarea.go (fulfills Req 2.1, 2.2)
Convert absolute start/end positions to row/column coordinates
Handle replacement across multiple rows if needed
Position cursor correctly after replacement
Add bounds checking and validation
[ ] 1.2. Test absolute replacement method: Write comprehensive tests for ReplaceRangeAbsolute (fulfills Req 2.1, 2.2)
- Test multi-row replacement scenarios
- Test edge cases (start/end of text, empty replacement)

2. Fix RestoreFromHistory implementation

[ ] 2.1. Update RestoreFromHistory logic: Modify RestoreFromHistory in packages/tui/internal/components/chat/editor.go (fulfills Req 1.1, 1.2, 1.3)
- Replace current row-based ReplaceRange calls with ReplaceRangeAbsolute
- Maintain existing reverse-order processing of attachments
- Remove SetCursorColumn calls that are now redundant
[ ] 2.2. Test history restoration: Write tests for corrected restoration behavior (fulfills Req 1.1, 1.2, 1.3)
- Test restoration of multi-line messages with attachments
- Test the exact scenario from the bug report
- Verify no attachment path duplication occurs

3. Integration and validation

[ ] 3.1. Create reproduction test: Add test that reproduces the exact bug scenario (fulfills Req 1.1, 1.2)
- Create message: "read this:\n@path/to/my/file.txt"
- Save to history and restore
- Verify output matches input exactly
[ ] 3.2. Test edge cases: Verify fix works for various attachment scenarios (fulfills Req 1.3, 2.3)
- Multiple attachments in multi-line text
- Attachments at line boundaries
- Empty lines between text and attachments
- Very long attachment paths

Jul 24 '25 01:07 rstacruz