navicore/seq-lisp

Fork 0

Refactor editor #55

Merged

navicore merged 6 commits from refactor-editor into main

2026-02-01 20:11:18 +00:00

navicore commented

2026-02-01 19:14:25 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-02-01 19:16:13 +00:00

(Migrated from github.com)

Pull Request Review: Refactor Editor

Overview

This PR refactors the vim-line editor by introducing structured types (EditorState and UndoEntry) to encapsulate editor state, replacing the previous approach of threading multiple stack values. The refactoring adds 783 lines while removing 199 lines, with significant new functionality including yank/paste operations, bracket matching, undo support, and improved word motion handling.

✅ Strengths

1. Excellent Architectural Improvement

The introduction of EditorState is a major win for maintainability:

Reduces stack juggling complexity throughout the codebase
Makes function signatures clearer: ( EditorState -- EditorState Bool ) vs ( String Int Int -- String Int Int Bool )
Provides a clean extension point for future features (fields can be added without changing all function signatures)

2. Well-Designed Updater Pattern

The updater functions (ed-with-buffer, ed-with-cursor, etc.) follow immutable data structure patterns effectively:

: ed-with-buffer ( EditorState String -- EditorState )
    over ed-cursor   
    2 pick ed-mode   
    3 pick ed-yank   
    ed-make          
    nip ;

This approach maintains immutability while providing ergonomic state updates.

3. Comprehensive Feature Additions

Bracket matching (% motion): Properly implements depth tracking for nested brackets
Yank/paste (y, p, P): Full clipboard functionality with register support
Undo (u in normal mode): Stack-based undo with depth limiting
Enhanced word motions: Respects word character boundaries (alphanumeric vs punctuation)

4. Good Documentation

Stack comments are thorough and helpful for understanding Seq's stack-based execution:

# Stack: EdState(3) Buffer(2) Cursor(1) NextPos(0)

⚠️ Issues & Concerns

1. Bug: Undo Stack Lost After Undo (HIGH)

Location: src/vim-line.seq:1184

undo-pop  # History Prompt HistPos EdState UndoStack Buffer Cursor
rot drop  # Drop old UndoStack: History Prompt HistPos EdState Buffer Cursor
# ... restore state ...
undo-empty  # Fresh undo stack after undo

Problem: After performing an undo, the undo stack is completely discarded and replaced with an empty stack. This means:

You can only undo once
You cannot undo multiple times in sequence
Redo is impossible to implement later

Expected behavior: The popped undo stack should be preserved so subsequent u commands can continue undoing previous changes.

Suggested fix:

# After undo-pop, keep the remaining stack
undo-pop  # ... EdState UndoStack Buffer Cursor
# Restore state with Buffer and Cursor
# Keep UndoStack (don't replace with undo-empty)
# Continue to vim-edit-inner with preserved UndoStack

2. Incomplete Feature: undo-limit Not Implemented (MEDIUM)

Location: src/vim-line.seq:134

: undo-limit ( UndoStack -- UndoStack )
    ;  # Empty function body - does nothing

Problem: The function undo-limit is called after every undo push but has an empty body. This means:

Undo stack grows unbounded (memory leak risk in long sessions)
The helper function undo-limit-loop (lines 138-154) is defined but never used
Comment says "Temporarily just return unchanged to debug" but this appears to be forgotten cleanup

Suggested fix:

: undo-limit ( UndoStack -- UndoStack )
    undo-max-depth undo-limit-loop ;

3. Potential Stack Overflow in Recursive Functions (MEDIUM)

Locations: Several helper functions use deep recursion:

undo-limit-loop (lines 138-154)
find-bracket-forward (lines 319-356)
find-bracket-backward (lines 360-397)

Problem: These recursive functions don't appear to be tail-recursive in Seq terms due to operations after the recursive call:

# In find-bracket-forward - not tail recursive
rot 1 i.add rot rot  # Increment Pos
find-bracket-forward  # Recursive call
# But earlier: swap scons after recursive call in undo-limit-loop

Risk: Very deep bracket nesting (1000+ levels) or large undo stacks could cause stack overflow.

Suggested mitigation: Consider iterative versions or verify that Seq's TCO handles these cases. Document maximum safe nesting depth.

4. Missing Ctrl+D/Ctrl+C Consistency (LOW)

Location: Multiple locations in key handlers

The PR changes EOF detection from string.empty? to "\x04" string equality (good!), but:

In src/repl.seq:9:

dup "\x04" string.equal? if
    # EOF (Ctrl+D or Ctrl+C)

In src/vim-line.seq:960-961:

dup key-ctrl-c i.= if drop "\x04" 0 mode-normal ed-with-buffer-cursor-mode false else
dup key-ctrl-d i.= if drop "\x04" 0 mode-normal ed-with-buffer-cursor-mode false else

Issue: The comment conflates Ctrl+C and Ctrl+D, but they have different semantics:

Ctrl+D = EOF signal (should finish input)
Ctrl+C = Interrupt signal (should cancel input)

Both returning "\x04" may be correct for this REPL, but it should be documented clearly. Consider whether Ctrl+C should return empty string or a different sentinel.

5. Tab Character Handling Added (MINOR)

Location: src/vim-line.seq:243

dup 10 i.= over 13 i.= or over 32 i.= or over 9 i.= or if
    # Newline (10), CR (13), space (32), or tab (9) - trim it

Change: Added tab (ASCII 9) to trailing whitespace trimming.

Question: Is this intentional for the REPL history? Tabs in Lisp code may be significant for some users. Consider whether this should only trim from history, not from the actual buffer content.

🔍 Code Quality Observations

Positive:

Consistent naming conventions: ed-*, undo-*, history-* prefixes
Good separation of concerns: Bracket matching is isolated, yank/paste is modular
Defensive programming: Bounds checking in bracket search functions

Suggestions:

Extract magic numbers to named constants:

: bracket-max-depth ( -- Int ) 1000 ;  # Add safety limit

Add error handling for bracket matching with extreme nesting
Test coverage: Ensure tests exist for:
- Multi-level undo (currently broken)
- Bracket matching with deep nesting
- Yank/paste edge cases (empty buffer, paste at boundaries)
- Word motions at buffer boundaries

🛡️ Security Considerations

No security concerns identified. The code operates on user input in a REPL context with appropriate bounds checking.

📊 Performance Considerations

Positive:

Immutable data structures prevent accidental mutation bugs
Yank register stored in EditorState (no global state)

Concerns:

Undo stack: Even with fixed undo-limit, 50 entries × (buffer + cursor) could be significant for large edits
Bracket matching: O(n) where n is distance between brackets - acceptable for typical code

🧪 Test Coverage

Status: Cannot verify without running tests, but areas needing coverage:

Undo functionality (especially multiple undo operations)
Bracket matching (nested, mismatched, edge cases)
Yank/paste (all operators: yy, y$, yw, p, P)
Word motions with punctuation boundaries
Undo stack limiting

📝 Recommendations

Must Fix (Before Merge):

Fix undo stack preservation (issue #1 above) - this is a functional bug
Implement undo-limit properly (issue #2) - memory leak potential

Should Fix (Before or Shortly After Merge):

Add test coverage for new features (undo, yank/paste, bracket matching)
Document or verify TCO behavior for recursive functions (issue #3)
Clarify Ctrl+C vs Ctrl+D semantics in comments (issue #4)

Nice to Have:

Extract magic numbers to named constants
Add integration test for long REPL sessions (memory leak testing)
Document maximum safe bracket nesting depth

Summary

This is a well-designed refactoring that significantly improves code maintainability and adds valuable vim features. The EditorState encapsulation is exactly the right architectural move for a stack-based language implementation.

However, there are two critical issues that need attention before merge:

Undo only works once (stack is lost)
Undo stack limiting is not implemented (memory leak)

Both are straightforward fixes. Once addressed, this will be an excellent improvement to the codebase.

Recommendation: Request changes for issues #1 and #2, then approve after fixes are verified.

Review generated for commit bc33dcd (Refactor editor)

# Pull Request Review: Refactor Editor ## Overview This PR refactors the vim-line editor by introducing structured types (`EditorState` and `UndoEntry`) to encapsulate editor state, replacing the previous approach of threading multiple stack values. The refactoring adds **783 lines** while removing **199 lines**, with significant new functionality including yank/paste operations, bracket matching, undo support, and improved word motion handling. ## ✅ Strengths ### 1. **Excellent Architectural Improvement** The introduction of `EditorState` is a major win for maintainability: - Reduces stack juggling complexity throughout the codebase - Makes function signatures clearer: `( EditorState -- EditorState Bool )` vs `( String Int Int -- String Int Int Bool )` - Provides a clean extension point for future features (fields can be added without changing all function signatures) ### 2. **Well-Designed Updater Pattern** The updater functions (`ed-with-buffer`, `ed-with-cursor`, etc.) follow immutable data structure patterns effectively: ```seq : ed-with-buffer ( EditorState String -- EditorState ) over ed-cursor 2 pick ed-mode 3 pick ed-yank ed-make nip ; ``` This approach maintains immutability while providing ergonomic state updates. ### 3. **Comprehensive Feature Additions** - **Bracket matching** (`%` motion): Properly implements depth tracking for nested brackets - **Yank/paste** (`y`, `p`, `P`): Full clipboard functionality with register support - **Undo** (`u` in normal mode): Stack-based undo with depth limiting - **Enhanced word motions**: Respects word character boundaries (alphanumeric vs punctuation) ### 4. **Good Documentation** Stack comments are thorough and helpful for understanding Seq's stack-based execution: ```seq # Stack: EdState(3) Buffer(2) Cursor(1) NextPos(0) ``` ## ⚠️ Issues & Concerns ### 1. **Bug: Undo Stack Lost After Undo (HIGH)** **Location:** `src/vim-line.seq:1184` ```seq undo-pop # History Prompt HistPos EdState UndoStack Buffer Cursor rot drop # Drop old UndoStack: History Prompt HistPos EdState Buffer Cursor # ... restore state ... undo-empty # Fresh undo stack after undo ``` **Problem:** After performing an undo, the undo stack is completely discarded and replaced with an empty stack. This means: - You can only undo once - You cannot undo multiple times in sequence - Redo is impossible to implement later **Expected behavior:** The popped undo stack should be preserved so subsequent `u` commands can continue undoing previous changes. **Suggested fix:** ```seq # After undo-pop, keep the remaining stack undo-pop # ... EdState UndoStack Buffer Cursor # Restore state with Buffer and Cursor # Keep UndoStack (don't replace with undo-empty) # Continue to vim-edit-inner with preserved UndoStack ``` ### 2. **Incomplete Feature: undo-limit Not Implemented (MEDIUM)** **Location:** `src/vim-line.seq:134` ```seq : undo-limit ( UndoStack -- UndoStack ) ; # Empty function body - does nothing ``` **Problem:** The function `undo-limit` is called after every undo push but has an empty body. This means: - Undo stack grows unbounded (memory leak risk in long sessions) - The helper function `undo-limit-loop` (lines 138-154) is defined but never used - Comment says "Temporarily just return unchanged to debug" but this appears to be forgotten cleanup **Suggested fix:** ```seq : undo-limit ( UndoStack -- UndoStack ) undo-max-depth undo-limit-loop ; ``` ### 3. **Potential Stack Overflow in Recursive Functions (MEDIUM)** **Locations:** Several helper functions use deep recursion: - `undo-limit-loop` (lines 138-154) - `find-bracket-forward` (lines 319-356) - `find-bracket-backward` (lines 360-397) **Problem:** These recursive functions don't appear to be tail-recursive in Seq terms due to operations after the recursive call: ```seq # In find-bracket-forward - not tail recursive rot 1 i.add rot rot # Increment Pos find-bracket-forward # Recursive call # But earlier: swap scons after recursive call in undo-limit-loop ``` **Risk:** Very deep bracket nesting (1000+ levels) or large undo stacks could cause stack overflow. **Suggested mitigation:** Consider iterative versions or verify that Seq's TCO handles these cases. Document maximum safe nesting depth. ### 4. **Missing Ctrl+D/Ctrl+C Consistency (LOW)** **Location:** Multiple locations in key handlers The PR changes EOF detection from `string.empty?` to `"\x04"` string equality (good!), but: **In `src/repl.seq:9`:** ```seq dup "\x04" string.equal? if # EOF (Ctrl+D or Ctrl+C) ``` **In `src/vim-line.seq:960-961`:** ```seq dup key-ctrl-c i.= if drop "\x04" 0 mode-normal ed-with-buffer-cursor-mode false else dup key-ctrl-d i.= if drop "\x04" 0 mode-normal ed-with-buffer-cursor-mode false else ``` **Issue:** The comment conflates Ctrl+C and Ctrl+D, but they have different semantics: - **Ctrl+D** = EOF signal (should finish input) - **Ctrl+C** = Interrupt signal (should cancel input) Both returning `"\x04"` may be correct for this REPL, but it should be documented clearly. Consider whether Ctrl+C should return empty string or a different sentinel. ### 5. **Tab Character Handling Added (MINOR)** **Location:** `src/vim-line.seq:243` ```seq dup 10 i.= over 13 i.= or over 32 i.= or over 9 i.= or if # Newline (10), CR (13), space (32), or tab (9) - trim it ``` **Change:** Added tab (ASCII 9) to trailing whitespace trimming. **Question:** Is this intentional for the REPL history? Tabs in Lisp code may be significant for some users. Consider whether this should only trim from history, not from the actual buffer content. ## 🔍 Code Quality Observations ### Positive: - **Consistent naming conventions**: `ed-*`, `undo-*`, `history-*` prefixes - **Good separation of concerns**: Bracket matching is isolated, yank/paste is modular - **Defensive programming**: Bounds checking in bracket search functions ### Suggestions: 1. **Extract magic numbers** to named constants: ```seq : bracket-max-depth ( -- Int ) 1000 ; # Add safety limit ``` 2. **Add error handling** for bracket matching with extreme nesting 3. **Test coverage**: Ensure tests exist for: - Multi-level undo (currently broken) - Bracket matching with deep nesting - Yank/paste edge cases (empty buffer, paste at boundaries) - Word motions at buffer boundaries ## 🛡️ Security Considerations **No security concerns identified.** The code operates on user input in a REPL context with appropriate bounds checking. ## 📊 Performance Considerations ### Positive: - Immutable data structures prevent accidental mutation bugs - Yank register stored in EditorState (no global state) ### Concerns: - **Undo stack:** Even with fixed `undo-limit`, 50 entries × (buffer + cursor) could be significant for large edits - **Bracket matching:** O(n) where n is distance between brackets - acceptable for typical code ## 🧪 Test Coverage **Status:** Cannot verify without running tests, but areas needing coverage: 1. Undo functionality (especially multiple undo operations) 2. Bracket matching (nested, mismatched, edge cases) 3. Yank/paste (all operators: `yy`, `y$`, `yw`, `p`, `P`) 4. Word motions with punctuation boundaries 5. Undo stack limiting ## 📝 Recommendations ### Must Fix (Before Merge): 1. **Fix undo stack preservation** (issue #1 above) - this is a functional bug 2. **Implement `undo-limit`** properly (issue #2) - memory leak potential ### Should Fix (Before or Shortly After Merge): 3. **Add test coverage** for new features (undo, yank/paste, bracket matching) 4. **Document or verify** TCO behavior for recursive functions (issue #3) 5. **Clarify** Ctrl+C vs Ctrl+D semantics in comments (issue #4) ### Nice to Have: 6. Extract magic numbers to named constants 7. Add integration test for long REPL sessions (memory leak testing) 8. Document maximum safe bracket nesting depth ## Summary This is a **well-designed refactoring** that significantly improves code maintainability and adds valuable vim features. The `EditorState` encapsulation is exactly the right architectural move for a stack-based language implementation. However, there are **two critical issues** that need attention before merge: 1. Undo only works once (stack is lost) 2. Undo stack limiting is not implemented (memory leak) Both are straightforward fixes. Once addressed, this will be an excellent improvement to the codebase. **Recommendation:** Request changes for issues #1 and #2, then approve after fixes are verified. --- *Review generated for commit `bc33dcd` (Refactor editor)*

claude[bot] commented

2026-02-01 19:30:53 +00:00

(Migrated from github.com)

Pull Request Review: Refactor Editor

Overview

This is a substantial refactoring that improves the vim-line editor architecture significantly. The changes total +787/-199 lines across 2 files, introducing an EditorState type to encapsulate editor state, undo functionality, and new vim operations.

Strengths

1. Excellent Architectural Improvement

EditorState encapsulation: The introduction of the EditorState union type is a major improvement, replacing manual stack threading of 3-4 separate values
Immutable update pattern: The ed-with-* updater functions follow a clean functional pattern
Multi-field updaters reduce stack manipulation complexity

2. Undo System Implementation

Well-designed undo stack with depth limiting (50 entries)
Smart triggering: only saves undo state for modifying operations
Clean separation via is-modifying-key predicates

3. Enhanced Vim Features

Bracket matching (% motion) with proper nesting depth tracking
Yank/paste (y, p, P) with full register support
Word motions improved to distinguish word chars from punctuation
Extended operators: dw, cw motions added

4. Code Quality

Comprehensive stack comments throughout
Consistent naming conventions
Good separation of concerns

Issues and Concerns

1. Potential Bug: EOF Detection Change (repl.seq:9-10)
Changed from string.empty? to checking for literal \x04 character. This could cause issues if Ctrl+D is typed mid-input. Please verify this is intentional.

2. Undo Stack Performance

undo-limit-loop traverses entire stack recursively on every push (O(n) operation)
Each undo entry stores full string copy
Consider using a counter or reducing max depth from 50 to 10-20 for REPL context

3. Stack Manipulation Complexity
Despite EditorState abstraction, some functions (like dw operator lines 655-677) have very complex stack manipulations. Consider extracting helper functions like delete-range.

4. Bracket Matching Edge Cases
Implementation doesn't handle strings or comments (e.g., opening paren in a string would be matched). Document this limitation or consider context-aware matching.

5. Tab Character Now Stripped
Tabs are now stripped from trailing whitespace. This is a behavior change that could affect existing workflows.

6. Missing Test Coverage
No test files were added/modified. Given complexity of refactor and new features, comprehensive tests are essential for EditorState updaters, undo/redo, bracket matching, and yank/paste.

Security and Performance

Security: Low risk. Undo stack has 50-entry limit. Control character handling with \x04 could be confusing if it appears in pasted content.

Performance:

Undo limiting is O(n) on every modification
Bracket matching is O(n) for deeply nested structures
String operations create intermediate copies

Summary

Overall Assessment: This is a high-quality refactor that significantly improves the codebase. The EditorState abstraction is excellent, and new features add substantial value.

Before merging, please address:

Verify EOF detection change (\x04 vs empty string)
Add test coverage for new functionality
Review undo stack performance (recursive limiting)
Consider extracting complex stack manipulations into helper functions
Document bracket matching limitations

Recommendation: Approve with minor changes. Core architecture is sound, but test coverage and EOF handling should be addressed.

Great work on this refactor! 🎉

## Pull Request Review: Refactor Editor ### Overview This is a substantial refactoring that improves the vim-line editor architecture significantly. The changes total +787/-199 lines across 2 files, introducing an EditorState type to encapsulate editor state, undo functionality, and new vim operations. ### Strengths **1. Excellent Architectural Improvement** - EditorState encapsulation: The introduction of the EditorState union type is a major improvement, replacing manual stack threading of 3-4 separate values - Immutable update pattern: The ed-with-* updater functions follow a clean functional pattern - Multi-field updaters reduce stack manipulation complexity **2. Undo System Implementation** - Well-designed undo stack with depth limiting (50 entries) - Smart triggering: only saves undo state for modifying operations - Clean separation via is-modifying-key predicates **3. Enhanced Vim Features** - Bracket matching (% motion) with proper nesting depth tracking - Yank/paste (y, p, P) with full register support - Word motions improved to distinguish word chars from punctuation - Extended operators: dw, cw motions added **4. Code Quality** - Comprehensive stack comments throughout - Consistent naming conventions - Good separation of concerns ### Issues and Concerns **1. Potential Bug: EOF Detection Change (repl.seq:9-10)** Changed from string.empty? to checking for literal \x04 character. This could cause issues if Ctrl+D is typed mid-input. Please verify this is intentional. **2. Undo Stack Performance** - undo-limit-loop traverses entire stack recursively on every push (O(n) operation) - Each undo entry stores full string copy - Consider using a counter or reducing max depth from 50 to 10-20 for REPL context **3. Stack Manipulation Complexity** Despite EditorState abstraction, some functions (like dw operator lines 655-677) have very complex stack manipulations. Consider extracting helper functions like delete-range. **4. Bracket Matching Edge Cases** Implementation doesn't handle strings or comments (e.g., opening paren in a string would be matched). Document this limitation or consider context-aware matching. **5. Tab Character Now Stripped** Tabs are now stripped from trailing whitespace. This is a behavior change that could affect existing workflows. **6. Missing Test Coverage** No test files were added/modified. Given complexity of refactor and new features, comprehensive tests are essential for EditorState updaters, undo/redo, bracket matching, and yank/paste. ### Security and Performance **Security**: Low risk. Undo stack has 50-entry limit. Control character handling with \x04 could be confusing if it appears in pasted content. **Performance**: - Undo limiting is O(n) on every modification - Bracket matching is O(n) for deeply nested structures - String operations create intermediate copies ### Summary **Overall Assessment**: This is a high-quality refactor that significantly improves the codebase. The EditorState abstraction is excellent, and new features add substantial value. **Before merging, please address**: 1. Verify EOF detection change (\x04 vs empty string) 2. Add test coverage for new functionality 3. Review undo stack performance (recursive limiting) 4. Consider extracting complex stack manipulations into helper functions 5. Document bracket matching limitations **Recommendation**: Approve with minor changes. Core architecture is sound, but test coverage and EOF handling should be addressed. Great work on this refactor! 🎉

claude[bot] commented

2026-02-01 19:55:45 +00:00

(Migrated from github.com)

PR Review: Refactor editor

Summary

This is a significant and well-executed refactor of the vim-line editor. The PR successfully introduces several major improvements while maintaining the core functionality. Overall, this is high-quality work with excellent code organization and thoughtful design decisions.

Stats: +798 lines, -199 lines across 2 files

✅ Strengths

1. Excellent Architectural Improvements

EditorState type: The introduction of the EditorState union type is a major win. It encapsulates buffer, cursor, mode, and yank_reg into a single value, dramatically simplifying stack management throughout the codebase.
Clean abstraction: The accessor/updater pattern (ed-buffer, ed-cursor, ed-with-buffer, etc.) provides a clean, functional interface for state manipulation.
Reduced cognitive load: Function signatures went from managing 3-4 separate stack values to passing a single EditorState, making the code much easier to reason about.

2. Feature Additions

Impressive set of new vim features:

Undo system: Full undo support with 50-level history (u command)
Yank/paste: y, p, P operators with proper register management
Bracket matching: % motion for navigating matching brackets (), [], {}
Enhanced operators: dw, cw for word-based operations
Better word navigation: Improved w motion that distinguishes between word chars, punctuation, and whitespace

3. Code Quality

Comprehensive comments: Stack comments clearly document the state at each step (e.g., # Stack: EdState(1) NewBuf(0))
Consistent naming: Good naming conventions throughout (ed-*, undo-*, normal-key-*)
Clean separation: Clear distinction between normal mode and insert mode handlers

⚠️ Issues & Concerns

1. Critical: Missing Test Coverage

Severity: HIGH

This is the biggest concern. A refactor of this magnitude (798 additions, 199 deletions) with zero visible test coverage is risky:

No tests for the new EditorState type
No tests for undo functionality
No tests for yank/paste operations
No tests for bracket matching
No regression tests to verify existing functionality still works

Recommendation:

# Create tests/seq/test_vim_line.seq with tests like:
- Test EditorState accessors/updaters
- Test undo-push/undo-pop edge cases (empty stack, max depth)
- Test bracket matching (nested brackets, mismatched, no brackets)
- Test yank/paste with empty register
- Test word motions on edge cases (empty string, all punctuation, etc.)

2. Potential Bug: Undo Stack Doesn't Preserve Yank Register

Location: vim-line.seq:1194-1198

# Lines 1194-1198 (in diff)
mode-normal ""
ed-make

When restoring from undo, the yank register is always set to "". This means:

User yanks text → yank register = "hello"
User makes edit → undo saves state with yank=""
User undoes → yank register is lost

Expected behavior: Undo should preserve the yank register or at least document this limitation.

Recommendation: Either:

Save yank register in UndoList type (add yank_reg: String field)
Document this as a known limitation in comments

3. Stack Management Complexity in Updaters

Location: vim-line.seq:52-68

The ed-with-cursor and ed-with-mode functions have subtle off-by-one patterns in their pick operations:

: ed-with-cursor ( EditorState Int -- EditorState )
    over ed-buffer   # EdState NewCur Buffer
    1 pick           # EdState NewCur Buffer NewCur (copy position 1)
    3 pick ed-mode   # EdState NewCur Buffer NewCur Mode
    4 pick ed-yank   # EdState NewCur Buffer NewCur Mode YankReg

: ed-with-mode ( EditorState Int -- EditorState )
    over ed-buffer   # EdState NewMode Buffer
    2 pick ed-cursor # EdState NewMode Buffer Cursor
    2 pick           # ... NewMode (position 2, not 3!)  <-- Note comment

The comments (position 2, not 3!) and (position 3, not 4!) suggest this was error-prone during development. While the code appears correct, this pattern is fragile.

Recommendation: Consider a helper macro or function to reduce boilerplate and potential for stack manipulation errors.

4. Bracket Matching: No Timeout for Large Files

Location: vim-line.seq:328-407

The find-bracket-forward and find-bracket-backward functions recursively scan the entire buffer. For a large input buffer (e.g., 10,000+ chars), this could cause:

Stack overflow (deep recursion)
Slow response time (no early exit for performance)

Recommendation:

Add a maximum search distance (e.g., 5000 chars)
Consider iterative implementation instead of recursive

5. Ctrl+D Behavior Change May Be Breaking

Location: repl.seq:9 and vim-line.seq:971

The PR changes Ctrl+D handling:

# Before:
dup string.empty? if  # EOF returns empty string

# After:  
dup "\x04" string.equal? if  # EOF returns literal "\x04"

This changes the contract of vim-edit. Callers now receive "\x04" instead of "" for EOF.

Recommendation:

Verify this doesn't break any upstream code in repl.seq
Document the new return value contract
Add test for Ctrl+D behavior

6. Undo Limit Algorithm Inefficiency

Location: vim-line.seq:138-160

The undo-limit-loop function rebuilds the entire undo list on every push:

: undo-limit-loop ( UndoStack Int -- UndoStack )
    # ... recursively rebuilds stack ...
    Make-UndoCons  # Creates new nodes even for unchanged entries

Performance impact:

Every edit triggers O(N) undo list reconstruction (N=50 max depth)
Creates unnecessary garbage (old list nodes)

Recommendation:

Only rebuild when limit is exceeded
Use a counter-based approach instead of recursive reconstruction

🔍 Minor Issues

7. Magic Numbers

: undo-max-depth ( -- Int ) 50 ;

While 50 is reasonable, consider making this configurable or documenting why 50 was chosen.

8. Missing Edge Case: Empty Buffer with `%`

What happens if user presses % on an empty buffer? The code doesn't explicitly handle this.

9. Tab Character in Whitespace Strip

Location: vim-line.seq:171

Good catch adding tab (9) to whitespace stripping! But verify this doesn't break anything that relied on the old behavior.

🛡️ Security Review

No security concerns identified. This is editor UI code with no:

External input parsing vulnerabilities
Command injection risks
Memory safety issues (Seq appears memory-safe)

📊 Performance Considerations

Undo stack: Reasonable 50-entry limit prevents unbounded memory growth ✓
Bracket matching: Could be slow on large buffers (see issue #4)
String operations: Multiple string.substring calls in yank/paste - acceptable for typical line editing
Undo limit: Inefficient O(N) rebuilding on every push (see issue #6)

🎯 Recommendations Summary

Must Fix Before Merge:

❗ Add comprehensive test coverage
❗ Fix or document undo yank register bug
❗ Verify Ctrl+D behavior change doesn't break callers

Should Fix:
4. Add max search distance to bracket matching
5. Optimize undo-limit to only rebuild when needed

Nice to Have:
6. Reduce stack manipulation boilerplate in updaters
7. Add edge case handling for empty buffers
8. Document undo-max-depth rationale

📝 Final Verdict

Recommendation: Approve with required changes

This is excellent work that significantly improves the editor architecture and adds valuable features. The EditorState abstraction is a major quality improvement. However, the lack of test coverage is a blocker for a refactor of this size.

Suggested next steps:

Add tests for new functionality (undo, yank/paste, bracket matching)
Add regression tests for existing features
Fix undo yank register bug
Verify Ctrl+D change works correctly with REPL

Once tests are added and the yank register issue is addressed, this will be ready to merge!

Great work overall! The refactor shows careful thought and clean execution. 🎉

# PR Review: Refactor editor ## Summary This is a **significant and well-executed refactor** of the vim-line editor. The PR successfully introduces several major improvements while maintaining the core functionality. Overall, this is high-quality work with excellent code organization and thoughtful design decisions. **Stats:** +798 lines, -199 lines across 2 files --- ## ✅ Strengths ### 1. **Excellent Architectural Improvements** - **EditorState type**: The introduction of the `EditorState` union type is a major win. It encapsulates `buffer`, `cursor`, `mode`, and `yank_reg` into a single value, dramatically simplifying stack management throughout the codebase. - **Clean abstraction**: The accessor/updater pattern (`ed-buffer`, `ed-cursor`, `ed-with-buffer`, etc.) provides a clean, functional interface for state manipulation. - **Reduced cognitive load**: Function signatures went from managing 3-4 separate stack values to passing a single `EditorState`, making the code much easier to reason about. ### 2. **Feature Additions** Impressive set of new vim features: - **Undo system**: Full undo support with 50-level history (`u` command) - **Yank/paste**: `y`, `p`, `P` operators with proper register management - **Bracket matching**: `%` motion for navigating matching brackets `()`, `[]`, `{}` - **Enhanced operators**: `dw`, `cw` for word-based operations - **Better word navigation**: Improved `w` motion that distinguishes between word chars, punctuation, and whitespace ### 3. **Code Quality** - **Comprehensive comments**: Stack comments clearly document the state at each step (e.g., `# Stack: EdState(1) NewBuf(0)`) - **Consistent naming**: Good naming conventions throughout (`ed-*`, `undo-*`, `normal-key-*`) - **Clean separation**: Clear distinction between normal mode and insert mode handlers --- ## ⚠️ Issues & Concerns ### 1. **Critical: Missing Test Coverage** **Severity: HIGH** This is the biggest concern. A refactor of this magnitude (798 additions, 199 deletions) with **zero visible test coverage** is risky: - No tests for the new `EditorState` type - No tests for undo functionality - No tests for yank/paste operations - No tests for bracket matching - No regression tests to verify existing functionality still works **Recommendation:** ```bash # Create tests/seq/test_vim_line.seq with tests like: - Test EditorState accessors/updaters - Test undo-push/undo-pop edge cases (empty stack, max depth) - Test bracket matching (nested brackets, mismatched, no brackets) - Test yank/paste with empty register - Test word motions on edge cases (empty string, all punctuation, etc.) ``` ### 2. **Potential Bug: Undo Stack Doesn't Preserve Yank Register** **Location:** `vim-line.seq:1194-1198` ```seq # Lines 1194-1198 (in diff) mode-normal "" ed-make ``` When restoring from undo, the yank register is always set to `""`. This means: - User yanks text → yank register = "hello" - User makes edit → undo saves state with yank="" - User undoes → yank register is lost **Expected behavior:** Undo should preserve the yank register or at least document this limitation. **Recommendation:** Either: 1. Save yank register in `UndoList` type (add `yank_reg: String` field) 2. Document this as a known limitation in comments ### 3. **Stack Management Complexity in Updaters** **Location:** `vim-line.seq:52-68` The `ed-with-cursor` and `ed-with-mode` functions have subtle off-by-one patterns in their `pick` operations: ```seq : ed-with-cursor ( EditorState Int -- EditorState ) over ed-buffer # EdState NewCur Buffer 1 pick # EdState NewCur Buffer NewCur (copy position 1) 3 pick ed-mode # EdState NewCur Buffer NewCur Mode 4 pick ed-yank # EdState NewCur Buffer NewCur Mode YankReg ``` vs ```seq : ed-with-mode ( EditorState Int -- EditorState ) over ed-buffer # EdState NewMode Buffer 2 pick ed-cursor # EdState NewMode Buffer Cursor 2 pick # ... NewMode (position 2, not 3!) <-- Note comment ``` The comments `(position 2, not 3!)` and `(position 3, not 4!)` suggest this was error-prone during development. While the code appears correct, this pattern is fragile. **Recommendation:** Consider a helper macro or function to reduce boilerplate and potential for stack manipulation errors. ### 4. **Bracket Matching: No Timeout for Large Files** **Location:** `vim-line.seq:328-407` The `find-bracket-forward` and `find-bracket-backward` functions recursively scan the entire buffer. For a large input buffer (e.g., 10,000+ chars), this could cause: - Stack overflow (deep recursion) - Slow response time (no early exit for performance) **Recommendation:** - Add a maximum search distance (e.g., 5000 chars) - Consider iterative implementation instead of recursive ### 5. **Ctrl+D Behavior Change May Be Breaking** **Location:** `repl.seq:9` and `vim-line.seq:971` The PR changes Ctrl+D handling: ```seq # Before: dup string.empty? if # EOF returns empty string # After: dup "\x04" string.equal? if # EOF returns literal "\x04" ``` This changes the contract of `vim-edit`. Callers now receive `"\x04"` instead of `""` for EOF. **Recommendation:** - Verify this doesn't break any upstream code in `repl.seq` - Document the new return value contract - Add test for Ctrl+D behavior ### 6. **Undo Limit Algorithm Inefficiency** **Location:** `vim-line.seq:138-160` The `undo-limit-loop` function rebuilds the entire undo list on every push: ```seq : undo-limit-loop ( UndoStack Int -- UndoStack ) # ... recursively rebuilds stack ... Make-UndoCons # Creates new nodes even for unchanged entries ``` **Performance impact:** - Every edit triggers O(N) undo list reconstruction (N=50 max depth) - Creates unnecessary garbage (old list nodes) **Recommendation:** - Only rebuild when limit is exceeded - Use a counter-based approach instead of recursive reconstruction --- ## 🔍 Minor Issues ### 7. **Magic Numbers** ```seq : undo-max-depth ( -- Int ) 50 ; ``` While 50 is reasonable, consider making this configurable or documenting why 50 was chosen. ### 8. **Missing Edge Case: Empty Buffer with `%`** What happens if user presses `%` on an empty buffer? The code doesn't explicitly handle this. ### 9. **Tab Character in Whitespace Strip** **Location:** `vim-line.seq:171` Good catch adding tab (9) to whitespace stripping! But verify this doesn't break anything that relied on the old behavior. --- ## 🛡️ Security Review **No security concerns identified.** This is editor UI code with no: - External input parsing vulnerabilities - Command injection risks - Memory safety issues (Seq appears memory-safe) --- ## 📊 Performance Considerations 1. **Undo stack**: Reasonable 50-entry limit prevents unbounded memory growth ✓ 2. **Bracket matching**: Could be slow on large buffers (see issue #4) 3. **String operations**: Multiple `string.substring` calls in yank/paste - acceptable for typical line editing 4. **Undo limit**: Inefficient O(N) rebuilding on every push (see issue #6) --- ## 🎯 Recommendations Summary **Must Fix Before Merge:** 1. ❗ Add comprehensive test coverage 2. ❗ Fix or document undo yank register bug 3. ❗ Verify Ctrl+D behavior change doesn't break callers **Should Fix:** 4. Add max search distance to bracket matching 5. Optimize undo-limit to only rebuild when needed **Nice to Have:** 6. Reduce stack manipulation boilerplate in updaters 7. Add edge case handling for empty buffers 8. Document undo-max-depth rationale --- ## 📝 Final Verdict **Recommendation: Approve with required changes** This is excellent work that significantly improves the editor architecture and adds valuable features. The `EditorState` abstraction is a major quality improvement. However, the **lack of test coverage** is a blocker for a refactor of this size. **Suggested next steps:** 1. Add tests for new functionality (undo, yank/paste, bracket matching) 2. Add regression tests for existing features 3. Fix undo yank register bug 4. Verify Ctrl+D change works correctly with REPL Once tests are added and the yank register issue is addressed, this will be ready to merge! --- **Great work overall! The refactor shows careful thought and clean execution.** 🎉

claude[bot] commented

2026-02-01 20:05:45 +00:00

(Migrated from github.com)

Code Review - PR #55: Refactor Editor

Summary

This PR refactors the vim-style line editor with significant architectural improvements. The changes demonstrate excellent attention to detail in stack-based programming, with comprehensive documentation and careful state management.

Strengths

Architecture & Design

EditorState encapsulation: Excellent refactoring to use a union type (EditorState) instead of threading 4 separate values through the stack. This significantly improves code maintainability and reduces cognitive load.
Undo system: Well-designed undo stack using a custom linked list (UndoList). The 50-entry depth limit is reasonable and prevents unbounded memory growth.
Multi-field updaters: Smart addition of ed-with-buffer-cursor and ed-with-buffer-cursor-mode helpers to reduce boilerplate for common update patterns.
Comprehensive stack comments: The detailed stack comments are invaluable for debugging and understanding stack-based code.

New Features

Bracket matching (%): Properly handles nested brackets with depth tracking
Yank/paste (y, p, P): Implements yank register with operators (yy, y$, yw) and paste before/after
Word motion improvements: Enhanced find-next-word to respect word character boundaries
Tab stripping: Added tab character (9) to whitespace trimming
Undo (u): Proper undo functionality with state saving before modifying operations

Code Quality

Consistent EOF handling: Changed from checking string.empty? to checking for Ctrl+D, making EOF signaling more explicit
Thorough documentation: Each major section has clear explanatory comments
Helper function reuse: Good use of helper functions like string-insert-string-at

Potential Issues & Suggestions

Minor Issues

Stack complexity in ed-with-cursor (vim-line.seq:57-64): The updater functions use pick with complex indices. While the comments are excellent, this pattern is inherently fragile.
Undo memory for yank register: The undo stack limits entries to 50, but yank_reg field could theoretically hold unbounded text. Likely acceptable for a line editor.
Bracket matching edge cases: Doesn't handle string literals containing brackets or comments. Acceptable for a line editor where context is limited.
Error handling in paste operations: Complex stack manipulations with multiple nip operations. Consider adding stack validation.
History navigation: Complex stack juggling makes verification difficult. Consider adding integration tests.

Performance Considerations

String operations: Many immutable operations. Fine for typical usage (<1000 chars)
Undo stack traversal: O(n²) worst case for n=50. Acceptable but could be optimized
Bracket matching: O(n) search is appropriate

Security Considerations

No security issues identified. The code properly handles untrusted input, has no buffer overflows, and no injection vulnerabilities.

Test Coverage

Observation: The PR modifies 806 lines but doesn't include new tests.

Recommendations:

Add unit tests for bracket matching, undo/redo, yank/paste, word motion
Add integration tests for history navigation with undo, mode transitions

Best Practices

Excellent stack discipline
Consistent naming conventions
Outstanding inline documentation
Proper functional style with immutable data structures

Conclusion

This is a high-quality refactor that significantly improves the editor's architecture and functionality. The main concern is lack of test coverage for the new features.

Recommendation: Approve with suggestion to add tests in a follow-up PR.

Review conducted by Claude Code

# Code Review - PR #55: Refactor Editor ## Summary This PR refactors the vim-style line editor with significant architectural improvements. The changes demonstrate excellent attention to detail in stack-based programming, with comprehensive documentation and careful state management. ## Strengths ### Architecture & Design - **EditorState encapsulation**: Excellent refactoring to use a union type (EditorState) instead of threading 4 separate values through the stack. This significantly improves code maintainability and reduces cognitive load. - **Undo system**: Well-designed undo stack using a custom linked list (UndoList). The 50-entry depth limit is reasonable and prevents unbounded memory growth. - **Multi-field updaters**: Smart addition of ed-with-buffer-cursor and ed-with-buffer-cursor-mode helpers to reduce boilerplate for common update patterns. - **Comprehensive stack comments**: The detailed stack comments are invaluable for debugging and understanding stack-based code. ### New Features - **Bracket matching (%)**: Properly handles nested brackets with depth tracking - **Yank/paste (y, p, P)**: Implements yank register with operators (yy, y$, yw) and paste before/after - **Word motion improvements**: Enhanced find-next-word to respect word character boundaries - **Tab stripping**: Added tab character (9) to whitespace trimming - **Undo (u)**: Proper undo functionality with state saving before modifying operations ### Code Quality - **Consistent EOF handling**: Changed from checking string.empty? to checking for Ctrl+D, making EOF signaling more explicit - **Thorough documentation**: Each major section has clear explanatory comments - **Helper function reuse**: Good use of helper functions like string-insert-string-at ## Potential Issues & Suggestions ### Minor Issues 1. **Stack complexity in ed-with-cursor** (vim-line.seq:57-64): The updater functions use pick with complex indices. While the comments are excellent, this pattern is inherently fragile. 2. **Undo memory for yank register**: The undo stack limits entries to 50, but yank_reg field could theoretically hold unbounded text. Likely acceptable for a line editor. 3. **Bracket matching edge cases**: Doesn't handle string literals containing brackets or comments. Acceptable for a line editor where context is limited. 4. **Error handling in paste operations**: Complex stack manipulations with multiple nip operations. Consider adding stack validation. 5. **History navigation**: Complex stack juggling makes verification difficult. Consider adding integration tests. ### Performance Considerations - String operations: Many immutable operations. Fine for typical usage (<1000 chars) - Undo stack traversal: O(n²) worst case for n=50. Acceptable but could be optimized - Bracket matching: O(n) search is appropriate ## Security Considerations No security issues identified. The code properly handles untrusted input, has no buffer overflows, and no injection vulnerabilities. ## Test Coverage **Observation**: The PR modifies 806 lines but doesn't include new tests. **Recommendations**: 1. Add unit tests for bracket matching, undo/redo, yank/paste, word motion 2. Add integration tests for history navigation with undo, mode transitions ## Best Practices - Excellent stack discipline - Consistent naming conventions - Outstanding inline documentation - Proper functional style with immutable data structures ## Conclusion This is a high-quality refactor that significantly improves the editor's architecture and functionality. The main concern is lack of test coverage for the new features. **Recommendation**: Approve with suggestion to add tests in a follow-up PR. --- *Review conducted by Claude Code*

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/seq-lisp!55

No description provided.

Rows
Columns

Refactor editor #55

Pull Request Review: Refactor Editor

Overview

✅ Strengths

1. Excellent Architectural Improvement

2. Well-Designed Updater Pattern

3. Comprehensive Feature Additions

4. Good Documentation

⚠️ Issues & Concerns

1. Bug: Undo Stack Lost After Undo (HIGH)

2. Incomplete Feature: undo-limit Not Implemented (MEDIUM)

3. Potential Stack Overflow in Recursive Functions (MEDIUM)

4. Missing Ctrl+D/Ctrl+C Consistency (LOW)

5. Tab Character Handling Added (MINOR)

🔍 Code Quality Observations

Positive:

Suggestions:

🛡️ Security Considerations

📊 Performance Considerations

Positive:

Concerns:

🧪 Test Coverage

📝 Recommendations

Must Fix (Before Merge):

Should Fix (Before or Shortly After Merge):

Nice to Have:

Summary

Pull Request Review: Refactor Editor

Overview

Strengths

Issues and Concerns

Security and Performance

Summary

PR Review: Refactor editor

Summary

✅ Strengths

1. Excellent Architectural Improvements

2. Feature Additions

3. Code Quality

⚠️ Issues & Concerns

1. Critical: Missing Test Coverage

2. Potential Bug: Undo Stack Doesn't Preserve Yank Register

3. Stack Management Complexity in Updaters

4. Bracket Matching: No Timeout for Large Files

5. Ctrl+D Behavior Change May Be Breaking

6. Undo Limit Algorithm Inefficiency

🔍 Minor Issues

7. Magic Numbers

8. Missing Edge Case: Empty Buffer with %

9. Tab Character in Whitespace Strip

🛡️ Security Review

📊 Performance Considerations

🎯 Recommendations Summary

📝 Final Verdict

Code Review - PR #55: Refactor Editor

Summary

Strengths

Architecture & Design

New Features

Code Quality

Potential Issues & Suggestions

Minor Issues

Performance Considerations

Security Considerations

Test Coverage

Best Practices

Conclusion

8. Missing Edge Case: Empty Buffer with `%`