refactor: architectural improvements from code review

Phase 6 architectural improvements:

- Remove production debug logging in TUI (Issue 6.1)
  - Replaced file-based debug_log with spdlog at trace level
  - Removed unnecessary includes (iostream, fstream)

- Extract Lua callback wrapper helper (Issue 6.2)
  - Added wrap_lua_callback() method to LuaApi class
  - Eliminates ~90 lines of duplicated callback wrapping code
  - Improves error context in Lua error messages

- Move yank state to KillRingManager (Issue 6.3)
  - Moved last_yank_start_ and last_yank_end_ from EditorCore
  - Added set_yank_range(), yank_start(), yank_end(), has_yank_state()
  - Better encapsulation of kill ring related state

- Added comprehensive REVIEW.md architectural documentation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

documentation/PLAN.md

@@ -213,4 +213,53 @@ Lumacs/
     *   Always run build, linting, and tests after each significant change. Identify the project's specific commands for these (e.g., `cmake --build build`, `ctest`).
     *   Ensure strict compiler warnings remain enabled and no new warnings are introduced.
 *   **User Interaction:**
-    *   If any ambiguity or complex decision arises during the refactoring process, clarify with the user.
+    *   If any ambiguity or complex decision arises during the refactoring process, clarify with the user.
+
+---
+
+## Phase 6: Architecture Review Improvements (December 2024)
+
+Based on the comprehensive architectural review in [REVIEW.md](./REVIEW.md), the following issues have been identified for resolution.
+
+### High Priority Issues
+
+| # | Issue | Status | Notes |
+|---|-------|--------|-------|
+| 6.1 | Remove production debug logging in TUI | 🟢 Completed | Replaced with spdlog (trace level) |
+| 6.2 | Extract Lua callback wrapper helper | 🟢 Completed | Added `wrap_lua_callback()` helper method |
+| 6.3 | Move yank state to KillRingManager | 🟢 Completed | Yank state now tracked in KillRingManager |
+| 6.4 | Extract ISearchManager from MinibufferManager | 🔴 Not Started | Reduce MinibufferManager complexity |
+| 6.5 | Extract common mode activation logic | 🔴 Not Started | TUI/GTK handle_editor_event duplication |
+
+### Medium Priority Issues
+
+| # | Issue | Status | Notes |
+|---|-------|--------|-------|
+| 6.6 | Create narrow interfaces for MacroManager/RectangleManager | 🔴 Not Started | Reduce EditorCore coupling |
+| 6.7 | Add user-visible Lua error reporting | 🔴 Not Started | Push errors to *Messages* buffer |
+| 6.8 | Create TUI header file for consistency | 🔴 Not Started | Match GtkEditor pattern |
+| 6.9 | Add plugin metadata/manifest support | 🔴 Not Started | Version, dependencies |
+| 6.10 | Create mock implementations for testing | 🔴 Not Started | IEditorNotifier, ICommandTarget mocks |
+
+### Low Priority Issues (Technical Debt)
+
+| # | Issue | Status | Notes |
+|---|-------|--------|-------|
+| 6.11 | Use unordered_map for Trie nodes | 🔴 Not Started | Performance optimization |
+| 6.12 | Split large files | 🔴 Not Started | lua_api.cpp, editor_core.cpp |
+| 6.13 | Clean up EditorCore constructor comments | 🔴 Not Started | After refactoring |
+
+### Legend
+
+- 🔴 Not Started
+- 🟡 In Progress
+- 🟢 Completed
+- ⏸️ Blocked
+
+### Change Log (Phase 6)
+
+| Date | Issue # | Description | Build Status |
+|------|---------|-------------|--------------|
+| 2025-12-04 | 6.1 | Replaced debug_log with spdlog in tui_editor.cpp | ✅ Pass |
+| 2025-12-04 | 6.2 | Added wrap_lua_callback() helper in lua_api.cpp | ✅ Pass |
+| 2025-12-04 | 6.3 | Moved yank state to KillRingManager | ✅ Pass |

documentation/REVIEW.md

@@ -0,0 +1,637 @@
+# Lumacs Architectural Review
+
+## Executive Summary
+
+Lumacs is a well-structured C++20 text editor with a **Facade/Mediator** pattern centered on `EditorCore`. The architecture demonstrates solid separation of concerns through interface segregation and dependency injection. However, there are several areas where the design could be improved for scalability, testability, and maintainability.
+
+---
+
+## 1. Separation of Concerns
+
+### Strengths
+
+**Interface Segregation is Well-Implemented**
+- Three core interfaces (`ICommandTarget`, `IEditorNotifier`, `IWindowManager`) effectively decouple components
+- `BufferManager` depends on interfaces (`i_editor_notifier.hpp:9-20`, `i_window_manager.hpp:11-24`), not concrete `EditorCore`
+- This enables dependency injection and breaks circular dependencies elegantly
+
+**Clear Delegation Pattern**
+- `EditorCore` (`editor_core.hpp:42-121`) delegates to specialized managers
+- Each operation routes to the appropriate manager (e.g., `kill_line()` → `KillRingManager`)
+
+### Issues
+
+**EditorCore Is Becoming a God Object**
+- `EditorCore` implements 3 interfaces with ~80 public methods (`editor_core.hpp:32-177`)
+- It holds 15+ member pointers (`editor_core.hpp:190-206`)
+- Constructor is 119 lines with complex initialization dependencies (`editor_core.cpp:19-119`)
+
+**Circular Dependency Workaround Is Fragile**
+```cpp
+// editor_core.cpp:105-106
+buffer_manager_ = std::make_unique<BufferManager>(*this, *this);
+window_manager_ = std::make_unique<WindowManager>(*this, *buffer_manager_);
+```
+The extensive comments in `editor_core.cpp:36-103` reveal the complexity of this workaround.
+
+### Recommendations
+
+1. **Split EditorCore into Coordinator + OperationsDelegate**
+   - `EditorCoordinator`: Manages lifecycle, initialization, and event routing
+   - `EditorOperations`: Implements `ICommandTarget` operations
+
+2. **Consider a Service Locator for Manager Access**
+   - Reduce the number of explicit dependencies in constructors
+   - Enable lazy initialization
+
+---
+
+## 2. Manager Proliferation
+
+### Current State: 11+ Managers
+
+| Manager | Dependencies | Responsibilities |
+|---------|--------------|------------------|
+| BufferManager | IEditorNotifier, IWindowManager | Buffer lifecycle, file I/O |
+| WindowManager | IEditorNotifier, BufferManager | Window layout tree |
+| KillRingManager | None | Clipboard-like ring buffer |
+| RegisterManager | None | Named text registers |
+| MacroManager | EditorCore | Keyboard macro recording |
+| RectangleManager | EditorCore | Rectangular region operations |
+| CommandSystem | EditorCore, MinibufferManager | Command registration/execution |
+| MinibufferManager | EditorCore, LuaApi, CompletionSystem | Modal input handling |
+| CompletionSystem | EditorCore | Completion candidates |
+| PluginManager | EditorCore, LuaApi | Plugin lifecycle |
+| KeyBindingManager | CommandSystem* | Key sequence handling (Trie) |
+| ThemeManager | None | Theme management |
+| ModelineManager | None | Status line composition |
+
+### Analysis
+
+**Well-Isolated Managers (Good)**
+- `KillRingManager`, `RegisterManager`, `ThemeManager`, `ModelineManager` have zero dependencies
+- These follow Single Responsibility Principle well
+
+**Over-Coupled Managers (Concern)**
+- `MacroManager`, `RectangleManager` take full `EditorCore&` reference
+- `MinibufferManager` depends on 3 systems (`minibuffer_manager.hpp:23`)
+
+**MinibufferManager Is Overloaded** (`minibuffer_manager.hpp:21-115`)
+- Handles multiple modes (Command, FilePath, BufferName, ThemeName, ISearch)
+- Manages history, completion, cursor position, search state
+- 115 lines of header with mixed responsibilities
+
+### Recommendations
+
+1. **Extract ISearchManager from MinibufferManager**
+   - `isearch_*` members and methods (`minibuffer_manager.hpp:100-111`) should be separate
+
+2. **Create MacroContext/RectangleContext Instead of EditorCore Reference**
+   - Define narrow interfaces for what these managers actually need
+
+3. **Consider Consolidation**:
+   - Merge `KillRingManager` and `RegisterManager` into a unified `ClipboardSystem`
+   - Both deal with text storage/retrieval
+
+---
+
+## 3. Lua Integration
+
+### Architecture
+
+```
+LuaApi (lua_api.hpp:23-101)
+  ├── sol::state lua_ (single global state)
+  ├── EditorCore* core_ (set via set_core())
+  ├── register_types() → exposes C++ usertypes
+  └── register_functions() → global Lua functions
+```
+
+### Strengths
+
+**Comprehensive Type Exposure** (`lua_api.cpp:390-898`)
+- Position, Range, Buffer, Window, EditorCore usertypes
+- Enums for ThemeElement, FontWeight, MinibufferMode
+- Callback support for buffer events (`lua_api.cpp:662-670`)
+
+**Command System Integration**
+```cpp
+// lua_api.cpp:764-823
+"bind_key", [this](EditorCore& core, std::string key, sol::object callback_or_cmd, ...)
+```
+- Keys can bind to either command names or Lua functions
+- Transparent interop between C++ commands and Lua callbacks
+
+### Issues
+
+**Single Global Lua State - Plugin Isolation Risk**
+```cpp
+// lua_api.hpp:89
+sol::state lua_;
+```
+- All plugins share the same Lua environment
+- One misbehaving plugin can corrupt global state
+- No sandboxing for untrusted plugins
+
+**LuaApi Constructor Before Core Assignment** (`lua_api.cpp:58-69`, `lua_api.cpp:71-74`)
+```cpp
+LuaApi::LuaApi() { lua_.open_libraries(...); }
+void LuaApi::set_core(EditorCore& core) { core_ = &core; setup_api(); }
+```
+- `set_core()` must be called before API is usable
+- No enforcement at compile time
+
+**Error Handling Is Log-Only** (`lua_api.cpp:83-86`)
+```cpp
+} catch (const sol::error& e) {
+    spdlog::error("Lua error loading {}: {}", path.string(), e.what());
+    return false;
+}
+```
+- Errors are logged but not propagated to UI
+- Users may not see why their config failed
+
+**Duplicated Callback Wrapper Code**
+- `lua_api.cpp:143-182`, `lua_api.cpp:777-810`, `lua_api.cpp:834-868` contain nearly identical callback wrapping logic
+
+### Recommendations
+
+1. **Plugin Sandboxing with Separate Environments**
+   ```cpp
+   sol::environment plugin_env(lua_, sol::create, lua_.globals());
+   ```
+
+2. **Compile-Time Core Requirement**
+   - Make `EditorCore&` a constructor parameter, or use RAII pattern
+
+3. **Extract Callback Wrapper Helper**
+   ```cpp
+   CommandResult wrap_lua_callback(sol::function& callback, CommandContext& context);
+   ```
+
+4. **User-Visible Error Reporting**
+   - Push Lua errors to `*Messages*` buffer or minibuffer
+
+---
+
+## 4. UI Layer Abstraction
+
+### Interface Design (`ui_interface.hpp:38-59`)
+
+```cpp
+class IEditorView {
+    virtual void init() = 0;
+    virtual void run() = 0;
+    virtual void handle_editor_event(EditorEvent event) = 0;
+    virtual void set_core(EditorCore* core) = 0;
+};
+```
+
+### Strengths
+
+**Clean Factory Pattern** (`gtk_editor.hpp:81`, `tui_editor.cpp:761-763`)
+```cpp
+std::unique_ptr<IEditorView> create_gtk_editor();
+std::unique_ptr<IEditorView> create_tui_editor();
+```
+
+**Conditional Compilation** (`main.cpp:52-66`)
+```cpp
+#ifdef LUMACS_WITH_GTK
+    if (!force_tui) { view = create_gtk_editor(); }
+#endif
+```
+
+### Issues
+
+**Event Handling Has Redundant Dispatching**
+
+Both `TuiEditor::handle_editor_event()` (`tui_editor.cpp:159-256`) and `GtkEditor::handle_editor_event()` have nearly identical switch statements for mode activation:
+
+```cpp
+// tui_editor.cpp:171-183
+} else if (event == EditorEvent::CommandMode) {
+    core_->minibuffer_manager().activate_minibuffer(
+        MinibufferMode::Command, ":",
+        [this](const std::string& input) { ... },
+        [this]() { core_->set_message("Cancelled"); }
+    );
+}
+```
+
+This is duplicated for BufferSwitchMode, KillBufferMode, FindFileMode, etc.
+
+**Raw Pointer to Core**
+```cpp
+// gtk_editor.hpp:43, tui_editor.cpp:35
+EditorCore* core_ = nullptr;
+```
+- No ownership semantics expressed
+- `set_core()` must be called before `init()`
+
+**TUI Implementation In .cpp**
+- `TuiEditor` class is defined entirely in `tui_editor.cpp:20-60`
+- Inconsistent with `GtkEditor` which has a header
+
+### Recommendations
+
+1. **Extract Common Mode Activation Logic**
+   ```cpp
+   class MinibufferModeHandler {
+       void handle_mode_event(EditorEvent event, EditorCore& core, ...);
+   };
+   ```
+
+2. **Use Non-Owning Smart Pointer**
+   ```cpp
+   std::reference_wrapper<EditorCore> core_;
+   ```
+
+3. **Create TUI Header for Consistency**
+   - Move class definition to `tui_editor.hpp`
+
+---
+
+## 5. Event System
+
+### Current Implementation (`ui_interface.hpp:12-32`, `editor_core.hpp:143-145`)
+
+```cpp
+enum class EditorEvent {
+    BufferModified, CursorMoved, ViewportChanged, WindowLayoutChanged,
+    WindowFocused, Message, CommandMode, BufferSwitchMode, ...
+};
+
+using EventCallback = std::function<void(EditorEvent)>;
+void on_event(EventCallback callback);
+```
+
+### Strengths
+
+- Simple, synchronous event dispatch
+- Clean separation via callback registration
+
+### Issues
+
+**No Event Data Payload**
+```cpp
+void EditorCore::emit_event(EditorEvent event) {
+    for (const auto& callback : event_callbacks_) {
+        callback(event);  // Only event type, no context
+    }
+}
+```
+- Observers must query `EditorCore` for related data
+- Race conditions possible in multi-threaded scenarios
+
+**O(n) Callback Iteration**
+```cpp
+std::vector<EventCallback> event_callbacks_;
+```
+- Fine for current use (typically 1 UI), but doesn't scale
+
+**Modal Events Mix Control and Data**
+- `CommandMode`, `ISearchMode` are events that trigger UI state changes
+- Different semantic level from `BufferModified`, `CursorMoved`
+
+### Recommendations
+
+1. **Add Event Payload**
+   ```cpp
+   struct EditorEventData {
+       EditorEvent type;
+       std::variant<BufferModifiedData, CursorMovedData, ...> payload;
+   };
+   ```
+
+2. **Consider Observer Pattern with Typed Handlers**
+   ```cpp
+   void on_buffer_modified(std::function<void(const BufferModifiedData&)> handler);
+   void on_cursor_moved(std::function<void(Position)> handler);
+   ```
+
+3. **Separate Modal Events into Different Mechanism**
+   - Use direct method calls for mode transitions
+   - Reserve events for state change notifications
+
+---
+
+## 6. Plugin/Extension System
+
+### Current Implementation (`plugin_manager.hpp:22-59`)
+
+```cpp
+class PluginManager {
+    void discover_plugins(const std::vector<std::filesystem::path>& plugin_dirs);
+    bool load_plugin(const std::string& name);
+    bool unload_plugin(const std::string& name);
+};
+```
+
+### Strengths
+
+- Simple file-based plugin discovery
+- Optional `on_load()`/`on_unload()` lifecycle hooks
+
+### Issues
+
+**No Plugin Metadata**
+```cpp
+struct Plugin {
+    std::string name;
+    std::filesystem::path path;
+    // No: version, dependencies, compatibility info
+};
+```
+
+**No Plugin Isolation**
+- All plugins run in shared `sol::state`
+- No capability restrictions
+- Plugins can access filesystem, network via Lua stdlib
+
+**No Plugin Dependency Resolution**
+- Plugins loaded in discovery order
+- No mechanism for plugin A to require plugin B
+
+**Unload Is Incomplete**
+```cpp
+void execute_on_unload(const Plugin& plugin);
+```
+- Calls Lua `on_unload()` but doesn't clean up:
+  - Registered commands
+  - Key bindings
+  - Event handlers
+
+### Recommendations
+
+1. **Plugin Manifest**
+   ```lua
+   -- plugin.lua
+   return {
+       name = "my-plugin",
+       version = "1.0.0",
+       requires = {"core >= 0.1.0"},
+       on_load = function() ... end
+   }
+   ```
+
+2. **Capability-Based Sandboxing**
+   ```cpp
+   sol::environment safe_env(lua_, sol::create);
+   safe_env["editor"] = core_subset;  // Limited API
+   ```
+
+3. **Plugin Registry for Cleanup**
+   - Track commands/bindings registered by each plugin
+   - Auto-cleanup on unload
+
+---
+
+## 7. State Management
+
+### Current State Distribution
+
+| Component | State Owned |
+|-----------|-------------|
+| BufferManager | `std::list<shared_ptr<Buffer>>` |
+| WindowManager | `shared_ptr<LayoutNode>`, `shared_ptr<Window>` |
+| Buffer | Content, undo stack, mark, styles |
+| Window | Cursor, viewport, buffer reference |
+| KillRingManager | `std::deque<std::string>` |
+| MinibufferManager | Mode, input, history, completion |
+| MacroManager | Recording state, macro content |
+
+### Issues
+
+**Shared Mutable State via shared_ptr**
+```cpp
+// buffer_manager.hpp:78
+std::list<std::shared_ptr<Buffer>> buffers_;
+```
+- Multiple windows can reference same Buffer
+- No synchronization primitives
+- Buffer modification from any path
+
+**Undo Is Buffer-Local Only**
+- No transaction semantics for multi-manager operations
+- Kill-then-yank is two unrelated operations from undo perspective
+
+**YankState in EditorCore** (`editor_core.hpp:187-188`)
+```cpp
+std::optional<Position> last_yank_start_;
+std::optional<Position> last_yank_end_;
+```
+- Implementation detail leaking into core class
+- Should be managed by KillRingManager or a YankContext
+
+### Recommendations
+
+1. **Consider Copy-on-Write for Buffer Content**
+   - Enables efficient snapshotting
+   - Safer for potential future threading
+
+2. **Centralized Undo Stack**
+   ```cpp
+   class UndoManager {
+       void begin_transaction();
+       void commit_transaction();
+       void rollback_transaction();
+   };
+   ```
+
+3. **Move Yank State to KillRingManager**
+   - Keep yank position tracking with kill ring logic
+
+---
+
+## 8. Testability
+
+### Current Test Structure (`tests/test_editor_core.cpp`)
+
+```cpp
+struct EditorCoreTest : public ::testing::Test {
+    lumacs::EditorCore core;  // Full system initialization
+};
+```
+
+### Strengths
+
+- Comprehensive test files for each manager
+- Integration tests in `test_integration.cpp`
+- Fixture-based setup with GoogleTest
+
+### Issues
+
+**Tests Create Full EditorCore**
+```cpp
+lumacs::EditorCore core;
+```
+- Every test initializes:
+  - All 11+ managers
+  - Lua VM
+  - Theme system
+  - Loads embedded defaults
+- Slow and couples tests to entire system
+
+**No Mock Implementations**
+- Interfaces exist (`ICommandTarget`, `IEditorNotifier`) but no mocks
+- Hard to test managers in isolation
+
+**LuaApi Test Depends on Disk** (`test_lua_api.cpp`)
+- Tests that load files can fail based on environment
+- No embedded test scripts
+
+**Comments Acknowledge Limitations** (`test_editor_core.cpp:103-106`)
+```cpp
+// Executing the macro will depend on process_key being called
+// which is hard to test without mocking keybinding_manager directly.
+// For now, just test recording state.
+```
+
+### Recommendations
+
+1. **Create Mock Implementations**
+   ```cpp
+   class MockEditorNotifier : public IEditorNotifier { ... };
+   class MockCommandTarget : public ICommandTarget { ... };
+   ```
+
+2. **Manager-Level Test Fixtures**
+   ```cpp
+   struct KillRingManagerTest : public ::testing::Test {
+       MockEditorNotifier notifier;
+       KillRingManager manager;  // No EditorCore needed!
+   };
+   ```
+
+3. **Embedded Lua Test Scripts**
+   ```cpp
+   static const char* TEST_LUA_SCRIPT = R"lua(...)lua";
+   ```
+
+---
+
+## 9. Performance Considerations
+
+### Current Bottlenecks
+
+**Key Processing Trie Lookup** (`keybinding.hpp:185-210`)
+```cpp
+struct TrieNode {
+    std::map<Key, std::unique_ptr<TrieNode>> children;
+};
+```
+- `std::map` means O(log n) per key in sequence
+- Fine for typical Emacs sequences, but `std::unordered_map` would be O(1)
+
+**Syntax Highlighting Per-Render** (`tui_editor.cpp:521-572`)
+```cpp
+const auto& styles = buffer.get_line_styles(buffer_line_idx);
+```
+- Styles retrieved and iterated per line, per frame
+- For large files, this could be expensive
+
+**Event Callback Vector Copy** (`editor_core.cpp:757-760`)
+```cpp
+for (const auto& callback : event_callbacks_) {
+    callback(event);
+}
+```
+- Safe iteration, but callbacks could modify the vector
+
+**No Line Caching in Buffer**
+- `buffer.line(idx)` constructs string view each time
+- No memoization for frequently accessed lines
+
+### Recommendations
+
+1. **Use `std::unordered_map` for Trie Nodes**
+   - With custom hash for `Key`
+
+2. **Cache Visible Lines' Styles**
+   - Invalidate on buffer modification in visible range
+
+3. **Consider Rope or Gap Buffer for Large Files**
+   - Current `std::vector<std::string>` is O(n) for mid-file insertions
+
+---
+
+## 10. Maintainability
+
+### Strengths
+
+- Consistent naming conventions
+- Doxygen-style comments on public interfaces
+- Clear header/implementation separation
+- Logical file organization
+
+### Issues
+
+**Large Files**
+- `lua_api.cpp`: 1031 lines
+- `editor_core.cpp`: 763 lines
+- `tui_editor.cpp`: 765 lines
+
+**Constructor Comments Reveal Design Debt** (`editor_core.cpp:27-103`)
+- 76 lines of comments explaining circular dependency workaround
+- Indicates architectural complexity
+
+**Inconsistent Header Guards**
+- Mix of `#pragma once` (good) throughout
+
+**Debug Code in Production**
+```cpp
+// tui_editor.cpp:14-15
+extern std::ofstream debug_log;
+std::ofstream debug_log("lumacs_debug.log");
+```
+- Creates log file unconditionally
+
+### Recommendations
+
+1. **Split Large Files**
+   - `lua_api.cpp` → `lua_types.cpp`, `lua_functions.cpp`, `lua_api.cpp`
+   - `editor_core.cpp` → core + operations
+
+2. **Remove Production Debug Logging**
+   - Use spdlog with runtime level control
+
+3. **Address Design Debt**
+   - Refactor circular dependency if comments exceed code
+
+---
+
+## Summary of Key Recommendations
+
+### High Priority
+
+1. **Split EditorCore** into Coordinator + Operations to address God Object anti-pattern
+2. **Extract ISearchManager** from MinibufferManager
+3. **Add Plugin Sandboxing** using Lua environments
+4. **Create Mock Implementations** for testability
+
+### Medium Priority
+
+5. **Unified Event Data** payloads for type-safe event handling
+6. **Common Mode Activation** handler shared between TUI/GTK
+7. **Plugin Manifest** system with dependency resolution
+8. **Extract Lua Callback Wrapper** to eliminate duplication
+
+### Low Priority (Technical Debt)
+
+9. **Performance optimizations** (Trie hash map, style caching)
+10. **Split large implementation files**
+11. **Production debug log cleanup**
+
+---
+
+## Conclusion
+
+Lumacs demonstrates a thoughtful architecture with proper interface segregation and dependency injection patterns. The main concerns are:
+
+1. **EditorCore complexity** - trending toward a God Object
+2. **Lua integration security** - shared state with no sandboxing
+3. **Test isolation** - full system initialization for each test
+4. **MinibufferManager overload** - too many responsibilities
+
+The foundation is solid, and the suggested improvements would enhance scalability without requiring a complete rewrite. The interface-based design already in place makes incremental refactoring feasible.

include/lumacs/editor_core.hpp

@@ -184,8 +184,6 @@ private:
     std::string last_message_;
     std::optional<std::chrono::steady_clock::time_point> message_clear_time_;
     std::vector<EventCallback> event_callbacks_;
-    std::optional<Position> last_yank_start_;
-    std::optional<Position> last_yank_end_;
 
     // Subsystems - Order matters for initialization based on dependencies
     // Declared in dependency order

include/lumacs/kill_ring_manager.hpp

@@ -10,6 +10,7 @@ namespace lumacs {
 
 /// @brief Implements an Emacs-like kill ring (clipboard history).
 /// The kill ring stores killed (cut) text in a circular buffer.
+/// Also tracks yank position state for yank-pop functionality.
 class KillRingManager {
 public:
     explicit KillRingManager(size_t max_size = 60);
@@ -34,10 +35,41 @@ public:
     /// @brief Clears the kill ring.
     void clear() { ring_.clear(); }
 
+    // === Yank State Tracking ===
+
+    /// @brief Record the start and end positions of a yank operation.
+    /// @param start The position where the yank started.
+    /// @param end The position where the yank ended.
+    void set_yank_range(Position start, Position end);
+
+    /// @brief Get the yank start position if a yank was performed.
+    [[nodiscard]] std::optional<Position> yank_start() const { return last_yank_start_; }
+
+    /// @brief Get the yank end position if a yank was performed.
+    [[nodiscard]] std::optional<Position> yank_end() const { return last_yank_end_; }
+
+    /// @brief Update just the yank end position (after yank-pop replaces text).
+    void set_yank_end(Position end) { last_yank_end_ = end; }
+
+    /// @brief Check if a yank operation has been recorded.
+    [[nodiscard]] bool has_yank_state() const {
+        return last_yank_start_.has_value() && last_yank_end_.has_value();
+    }
+
+    /// @brief Clear the yank state (e.g., after a non-yank command).
+    void clear_yank_state() {
+        last_yank_start_.reset();
+        last_yank_end_.reset();
+    }
+
 private:
     std::deque<std::string> ring_;
     size_t max_size_;
     bool last_action_was_kill_ = false; // Tracks if the last push was a kill or append
+
+    // Yank position tracking (moved from EditorCore)
+    std::optional<Position> last_yank_start_;
+    std::optional<Position> last_yank_end_;
 };
 
 } // namespace lumacs

include/lumacs/lua_api.hpp

@@ -87,17 +87,24 @@ public:
 
 private:
     sol::state lua_;
-    EditorCore* core_ = nullptr; 
+    EditorCore* core_ = nullptr;
     std::map<std::string, sol::function> legacy_key_bindings_;  // For backward compatibility
 
     /// Initialize Lua state and libraries.
     void setup_api();
-    
+
     /// Register C++ user types (usertypes) with Sol2.
     void register_types();
-    
+
     /// Register global C++ functions in Lua.
     void register_functions();
+
+    /// @brief Create a CommandFunction that wraps a Lua callback.
+    /// This eliminates code duplication across bind_key and register_command.
+    /// @param callback The Lua function to wrap.
+    /// @param error_context Description of the context for error messages.
+    /// @return A CommandFunction suitable for CommandSystem::register_command.
+    CommandFunction wrap_lua_callback(sol::function callback, const std::string& error_context);
 };
 
 } // namespace lumacs

src/editor_core.cpp

@@ -20,8 +20,6 @@ EditorCore::EditorCore() :
     last_message_(),
     message_clear_time_(),
     event_callbacks_(),
-    last_yank_start_(),
-    last_yank_end_(),
     theme_manager_(),
     config_(),
     modeline_manager_member_(), // Direct member
@@ -503,12 +501,12 @@ void EditorCore::copy_region_as_kill() {
 }
 
 void EditorCore::yank() {
-    if (kill_ring_manager_->empty()) { // Delegate to manager
+    if (kill_ring_manager_->empty()) {
         set_message("Kill ring is empty");
         return;
     }
 
-    std::string text = kill_ring_manager_->current(); // Delegate to manager
+    std::string text = kill_ring_manager_->current();
     if (text.empty()) {
         return;
     }
@@ -516,16 +514,11 @@ void EditorCore::yank() {
     auto& buf = *buffer_manager_->active_buffer();
     auto cursor = window_manager_->active_window()->cursor();
 
-    // Save yank start position
-    last_yank_start_ = cursor;
-
     // Insert the text
     buf.insert(cursor, text);
 
     // Calculate new cursor position after insertion
     Position new_cursor = cursor;
-
-    // Count newlines in text
     size_t newline_count = std::count(text.begin(), text.end(), '\n');
 
     if (newline_count > 0) {
@@ -538,7 +531,8 @@ void EditorCore::yank() {
         new_cursor.column += text.size();
     }
 
-    last_yank_end_ = new_cursor;
+    // Save yank range in KillRingManager for yank-pop
+    kill_ring_manager_->set_yank_range(cursor, new_cursor);
     window_manager_->active_window()->set_cursor(new_cursor);
 
     emit_event(EditorEvent::BufferModified);
@@ -548,26 +542,26 @@ void EditorCore::yank() {
 }
 
 void EditorCore::yank_pop() {
-    if (kill_ring_manager_->empty()) { // Delegate to manager
+    if (kill_ring_manager_->empty()) {
         set_message("Kill ring is empty");
         return;
     }
 
-    if (!last_yank_start_.has_value() || !last_yank_end_.has_value()) {
+    if (!kill_ring_manager_->has_yank_state()) {
         set_message("Previous command was not a yank");
         return;
     }
 
     // Delete the previously yanked text
     auto& buf = *buffer_manager_->active_buffer();
-    Range yank_range = {last_yank_start_.value(), last_yank_end_.value()};
+    Range yank_range = {kill_ring_manager_->yank_start().value(), kill_ring_manager_->yank_end().value()};
     buf.erase(yank_range);
 
     // Get previous entry in kill ring
-    std::string text = kill_ring_manager_->previous(); // Delegate to manager
+    std::string text = kill_ring_manager_->previous();
 
     // Restore cursor to yank start
-    auto cursor = last_yank_start_.value();
+    auto cursor = kill_ring_manager_->yank_start().value();
     window_manager_->active_window()->set_cursor(cursor);
 
     // Insert new text
@@ -585,7 +579,8 @@ void EditorCore::yank_pop() {
         new_cursor.column += text.size();
     }
 
-    last_yank_end_ = new_cursor;
+    // Update yank end position
+    kill_ring_manager_->set_yank_end(new_cursor);
     window_manager_->active_window()->set_cursor(new_cursor);
 
     emit_event(EditorEvent::BufferModified);

src/kill_ring_manager.cpp

@@ -38,7 +38,7 @@ std::string KillRingManager::previous() {
     if (ring_.empty()) {
         return "";
     }
-    
+
     // Rotate the ring: move back to front (conceptual 'previous')
     std::string back_item = ring_.back();
     ring_.pop_back();
@@ -48,4 +48,9 @@ std::string KillRingManager::previous() {
     return ring_.front();
 }
 
+void KillRingManager::set_yank_range(Position start, Position end) {
+    last_yank_start_ = start;
+    last_yank_end_ = end;
+}
+
 } // namespace lumacs

src/lua_api.cpp

@@ -55,6 +55,43 @@ private:
     sol::function provider_;
 };
 
+CommandFunction LuaApi::wrap_lua_callback(sol::function callback, const std::string& error_context) {
+    return [callback, error_context, this](CommandContext& context) -> CommandResult {
+        try {
+            // Pass args from context to Lua function
+            sol::table args_table = get_lua_state().create_table();
+            const auto& args = context.get_args();
+            for (size_t i = 0; i < args.size(); ++i) {
+                args_table[i + 1] = args[i];
+            }
+            auto result = callback(args_table);
+            if (result.valid()) {
+                if (result.get_type() == sol::type::table) {
+                    sol::table res_table = result;
+                    CommandStatus status = CommandStatus::Success;
+                    std::string message;
+
+                    if (res_table["success"].valid()) {
+                        status = res_table["success"].get<bool>() ? CommandStatus::Success : CommandStatus::Failure;
+                    }
+                    if (res_table["message"].valid()) {
+                        message = res_table["message"];
+                    }
+                    return CommandResult{status, message};
+                } else if (result.get_type() == sol::type::string) {
+                    return CommandResult{CommandStatus::Success, result.get<std::string>()};
+                } else {
+                    return CommandResult{CommandStatus::Success, ""};
+                }
+            } else {
+                return CommandResult{CommandStatus::Success, ""};
+            }
+        } catch (const sol::error& e) {
+            return CommandResult{CommandStatus::Failure, "Lua error in " + error_context + ": " + std::string(e.what())};
+        }
+    };
+}
+
 LuaApi::LuaApi() {
     lua_.open_libraries(
         sol::lib::base,
@@ -134,51 +171,17 @@ bool LuaApi::load_init_file() {
 
 void LuaApi::bind_key(std::string key, sol::function callback, std::string description) {
     spdlog::debug("Registering key binding: {}", key);
-    
+
     // Create a unique command name for the Lua function
-    // This is a simple way; a more robust system might handle conflicts or namespaces
-    std::string command_name = "lua_cmd_" + key; // Use key itself as part of name
+    std::string command_name = "lua_cmd_" + key;
 
     // Register a C++ CommandFunction that wraps the Lua callback
-    core_->command_system().register_command(command_name, 
-        [callback, this](CommandContext& context) -> CommandResult { // Added 'this' to capture
-            try {
-                // Pass args from context to Lua function
-                sol::table args_table = get_lua_state().create_table(); // Use get_lua_state()
-                const auto& args = context.get_args(); // Use getter
-                for (size_t i = 0; i < args.size(); ++i) {
-                    args_table[i + 1] = args[i];
-                }
-                auto result = callback(args_table);
-                if (result.valid()) {
-                    if (result.get_type() == sol::type::table) {
-                        sol::table res_table = result;
-                        lumacs::CommandStatus status = lumacs::CommandStatus::Success; // Namespace fix
-                        std::string message = "";
-                        
-                        if (res_table["success"].valid()) {
-                            status = res_table["success"].get<bool>() ? lumacs::CommandStatus::Success : lumacs::CommandStatus::Failure;
-                        }
-                        if (res_table["message"].valid()) {
-                            message = res_table["message"];
-                        }
-                        return CommandResult{status, message};
-                    } else if (result.get_type() == sol::type::string) {
-                        std::string message = result.get<std::string>();
-                        return CommandResult{lumacs::CommandStatus::Success, message};
-                    } else {
-                        return CommandResult{lumacs::CommandStatus::Success, ""};
-                    }
-                } else {
-                    return CommandResult{lumacs::CommandStatus::Success, ""};
-                }
-            } catch (const sol::error& e) {
-                return CommandResult{lumacs::CommandStatus::Failure, "Lua error in key binding callback: " + std::string(e.what())};
-            }
-        },
-        description, // Use original description
-        false,       // Not interactive directly via spec, Lua callback handles interactivity
-        ""           // No interactive spec for this wrapper command
+    core_->command_system().register_command(
+        command_name,
+        wrap_lua_callback(callback, "key binding '" + key + "'"),
+        description,
+        false,  // Not interactive directly via spec
+        ""      // No interactive spec
     );
 
     // Now bind the key to the newly registered C++ command's name
@@ -766,55 +769,21 @@ void LuaApi::register_types() {
                 // Bind directly to an existing command name
                 std::string cmd_name = callback_or_cmd.as<std::string>();
                 core.keybinding_manager().bind(KeySequence(key), cmd_name, description.value_or(""));
-            } 
+            }
             else if (callback_or_cmd.is<sol::function>()) {
                 // Create a unique command name for the Lua function
-                std::string command_name = "lua_cmd_" + key; // Use key itself as part of name
+                std::string command_name = "lua_cmd_" + key;
                 sol::function callback = callback_or_cmd.as<sol::function>();
 
-                // Register a C++ CommandFunction that wraps the Lua callback
-                core.command_system().register_command(command_name, 
-                    [callback, this](CommandContext& context) -> lumacs::CommandResult { // Namespace fix
-                        try {
-                            // Pass args from context to Lua function
-                            sol::table args_table = get_lua_state().create_table(); // Use get_lua_state()
-                            const auto& args = context.get_args(); // Use getter
-                            for (size_t i = 0; i < args.size(); ++i) {
-                                args_table[i + 1] = args[i];
-                            }
-                            auto result = callback(args_table);
-                            if (result.valid()) {
-                                if (result.get_type() == sol::type::table) {
-                                    sol::table res_table = result;
-                                    lumacs::CommandStatus status = lumacs::CommandStatus::Success; // Namespace fix
-                                    std::string message = "";
-                                    
-                                    if (res_table["success"].valid()) {
-                                        status = res_table["success"].get<bool>() ? lumacs::CommandStatus::Success : lumacs::CommandStatus::Failure;
-                                    }
-                                    if (res_table["message"].valid()) {
-                                        message = res_table["message"];
-                                    }
-                                    return lumacs::CommandResult{status, message};
-                                } else if (result.get_type() == sol::type::string) {
-                                    std::string message = result.get<std::string>();
-                                    return lumacs::CommandResult{lumacs::CommandStatus::Success, message};
-                                } else {
-                                    return lumacs::CommandResult{lumacs::CommandStatus::Success, ""};
-                                }
-                            } else {
-                                return lumacs::CommandResult{lumacs::CommandStatus::Success, ""};
-                            }
-                        } catch (const sol::error& e) {
-                            return lumacs::CommandResult{lumacs::CommandStatus::Failure, "Lua error in key binding callback: " + std::string(e.what())};
-                        }
-                    },
-                    description.value_or(""), // Use original description
-                    false,       // Not interactive directly via spec, Lua callback handles interactivity
-                    ""           // No interactive spec for this wrapper command
+                // Register using the helper function
+                core.command_system().register_command(
+                    command_name,
+                    wrap_lua_callback(callback, "key binding '" + key + "'"),
+                    description.value_or(""),
+                    false,  // Not interactive
+                    ""      // No interactive spec
                 );
 
-                // Now bind the key to the newly registered C++ command's name
                 core.keybinding_manager().bind(KeySequence(key), command_name, description.value_or(""));
             }
             else {
@@ -830,44 +799,15 @@ void LuaApi::register_types() {
                     aliases.push_back(pair.second.as<std::string>());
                 }
             }
-            
-            CommandFunction command_func = [this, func](CommandContext& context) -> lumacs::CommandResult { // Namespace fix
-                try {
-                    // Pass args from context to Lua function
-                    sol::table args_table = get_lua_state().create_table(); // Use get_lua_state()
-                    const auto& args = context.get_args(); // Use getter
-                    for (size_t i = 0; i < args.size(); ++i) {
-                        args_table[i + 1] = args[i];
-                    }
-                    auto result = func(args_table);
-                    if (result.valid()) {
-                        if (result.get_type() == sol::type::table) {
-                            sol::table res_table = result;
-                            lumacs::CommandStatus status = lumacs::CommandStatus::Success; // Namespace fix
-                            std::string message = "";
-                            
-                            if (res_table["success"].valid()) {
-                                status = res_table["success"].get<bool>() ? lumacs::CommandStatus::Success : lumacs::CommandStatus::Failure;
-                            }
-                            if (res_table["message"].valid()) {
-                                message = res_table["message"];
-                            }
-                            return lumacs::CommandResult{status, message};
-                        } else if (result.get_type() == sol::type::string) {
-                            std::string message = result.get<std::string>();
-                            return lumacs::CommandResult{lumacs::CommandStatus::Success, message};
-                        } else {
-                            return lumacs::CommandResult{lumacs::CommandStatus::Success, ""};
-                        }
-                    } else {
-                        return lumacs::CommandResult{lumacs::CommandStatus::Success, ""};
-                    }
-                } catch (const sol::error& e) {
-                    return lumacs::CommandResult{lumacs::CommandStatus::Failure, "Lua error: " + std::string(e.what())};
-                }
-            };
-            
-            core.command_system().register_command(name, command_func, description, interactive.value_or(true), interactive_spec.value_or(""), aliases);
+
+            core.command_system().register_command(
+                name,
+                wrap_lua_callback(func, "command '" + name + "'"),
+                description,
+                interactive.value_or(true),
+                interactive_spec.value_or(""),
+                aliases
+            );
         },
 
 

src/tui_editor.cpp

@@ -2,17 +2,12 @@
 #include "lumacs/editor_core.hpp"
 #include "lumacs/lua_api.hpp"
 #include <ncurses.h>
-#include <iostream>
-#include <fstream>
 #include <memory>
 #include <chrono>
 #include <string>
 #include <sstream>
 #include <algorithm>
-
-// Global debug log
-extern std::ofstream debug_log;
-std::ofstream debug_log("lumacs_debug.log");
+#include <spdlog/spdlog.h>
 
 using namespace lumacs;
 
@@ -92,7 +87,7 @@ void TuiEditor::init() {
     int content_width = width_ - line_number_width;
     core_->set_viewport_size(content_width, content_height);
     
-    debug_log << "ncurses editor initialized: " << width_ << "x" << height_ << std::endl;
+    spdlog::debug("ncurses editor initialized: {}x{}", width_, height_);
 }
 
 void TuiEditor::run() {
@@ -116,8 +111,8 @@ void TuiEditor::run() {
             int line_number_width = show_line_numbers ? core_->config().get<int>("line_number_width", 6) : 0;
             int content_width = width_ - line_number_width;
             core_->set_viewport_size(content_width, content_height);
-            debug_log << "Screen resized to: " << width_ << "x" << height_ << std::endl;
-            debug_log << "Content area: " << content_width << "x" << content_height << std::endl;
+            spdlog::debug("Screen resized to: {}x{}", width_, height_);
+            spdlog::debug("Content area: {}x{}", content_width, content_height);
             // Force cursor to be visible after resize, as it implies movement.
             last_cursor_move_time_ = std::chrono::steady_clock::now();
             cursor_visible_ = true;
@@ -271,8 +266,7 @@ int TuiEditor::get_attributes_for_face(const std::string& face_name) {
 
 /// Convert ncurses key code to our key name format
 std::string TuiEditor::resolve_key(int ch) {
-    debug_log << "=== NCURSES INPUT DEBUG ===" << std::endl;
-    debug_log << "Raw key code: " << ch << " (0x" << std::hex << ch << std::dec << ")" << std::endl;
+    spdlog::trace("NCURSES INPUT: Raw key code: {} (0x{:x})", ch, ch);
     
     if (ch == 27) return "Escape";
     if (ch == '\n' || ch == '\r' || ch == KEY_ENTER) return "Return";
@@ -337,11 +331,11 @@ bool TuiEditor::handle_input(int ch) {
     }
     
     if (key_name.empty()) {
-        debug_log << "Empty key name, ignoring input" << std::endl;
+        spdlog::trace("Empty key name, ignoring input");
         return false;
     }
 
-    debug_log << "Resolved key: " << key_name << std::endl;
+    spdlog::trace("Resolved key: {}", key_name);
 
     // Handle Minibuffer Input Logic first
     if (core_->minibuffer_manager().is_active()) {
@@ -496,10 +490,8 @@ void TuiEditor::render_window(std::shared_ptr<Window> window, int x, int y, int
     auto [start_line, end_line] = window->visible_line_range();
     bool is_active = (window == core_->active_window());
     
-    debug_log << "Render window at " << x << "," << y << " size " << width << "x" << height 
-              << " viewport=" << start_line << "-" << end_line 
-              << " cursor=(" << cursor.line << "," << cursor.column << ")"
-              << " active=" << is_active << std::endl;
+    spdlog::trace("Render window at {},{} size {}x{} viewport={}-{} cursor=({},{}) active={}",
+                  x, y, width, height, start_line, end_line, cursor.line, cursor.column, is_active);
     
     // Render buffer lines
     for (int screen_y = 0; screen_y < content_height && start_line + screen_y < end_line; ++screen_y) {