docs/internal/code-review.md
Pending code quality improvements and refactoring opportunities.
Note: file/line references are best-effort and can drift as the code changes; treat line numbers as approximate.
render runs ~450 lines covering layout calculation, plugin hook firing, file explorer rendering, and status/prompt UI. The amount of responsibility in one function hurts readability and testing; factoring into helpers (layout, plugin hook prep, explorer rendering, status/prompt rendering) would make regressions easier to spot.Action::from_str is a ~260-line string-to-enum match. A data-driven table would reduce boilerplate and avoid missing new actions when added to the enum.get_all_commands is a ~540-line literal list in one function. This is brittle to maintain (hard to diff/review additions) and couples command metadata to code; consider a data table or config-driven source with tests for completeness.highlight_config is ~450 lines of repetitive per-language setup; moving to a data table (language -> queries/config) would reduce duplication and make it harder to forget highlight names when adding languages.default_menus is a 420-line literal definition. Consider moving menu data to structured config or a table to make changes easier to diff/test and to avoid bloating code with data.ensure_visible runs ~230 lines of layout math and clamping in one method; breaking into smaller helpers (e.g., horizontal/vertical logic, scroll computations) would make correctness checks and future changes safer.format_action spans ~200 lines mirroring from_str; another sign a data-driven action registry would reduce duplication and risk of drift.next is a 160+ line iterator step that handles multiple line types and wrapping; decomposing per-case helpers would make it easier to verify wrapping and newline behaviors.get_text_range_mut is a 160+ line method with intertwined chunk handling, file streaming, and cache management. The dense control flow increases the risk of off-by-one or range bugs; extract helpers per buffer variant (loaded vs unloaded) and centralize bounds checks.pending_responses.lock().unwrap() (and other mutex unwrap()s) in ops will panic if the mutex is poisoned by a plugin panic. These should handle poisoning and return an error to the plugin rather than crashing the runtime.view.tree().get_node(node_id).expect("Node should exist"). A desynced tree (e.g., after FS errors or refresh races) will panic the UI; prefer graceful fallback or placeholder row.expand_node uses self.get_node(id).unwrap() while doing async FS reads. If the node was removed between calls, this panics instead of returning an IO error; return a proper error for missing nodes to avoid crashing the explorer.render_status pulls cursor info with ad-hoc line iteration and cached line numbers in a long method; consider extracting helpers for cursor position and diagnostics summary to reduce the ~200-line render method complexity.primary()/primary_mut() use expect("Primary cursor should always exist"). If a bug ever removes the primary cursor (e.g., during multi-cursor deletion), the editor will panic. Prefer returning an error or recreating a primary cursor to keep the UI alive.unwrap() on child pointers. Any tree corruption (e.g., from earlier logic bugs) will panic. Consider defensive checks or debug assertions gated for release builds to avoid crashing the editor.ensure_layout returns self.layout.as_ref().unwrap(). If callers forget to call ensure_layout before get_layout usage, rendering will panic. Consider returning Option<&Layout> or asserting via a Result to make misuse harder in production.unwrap() optional highlight colors/positions when overlays and syntax spans overlap. If the lookups ever return None (e.g., missing semantic span), the renderer will panic; handle None with defaults to keep the UI resilient.selected_text().unwrap() in selection paths; production prompt operations should avoid panics when no selection exists (e.g., racey UI updates or plugin-driven prompt changes).ensure_cursors_visible unwraps min/max cursor positions; if called with an empty cursor list (due to upstream logic bugs), it will panic. Guard against empty input to keep scrolling resilient..max().unwrap_or(0) on suggestion lists; if any suggestion text/keybinding is extremely wide, width computation and rendering alignment are packed into the main render function. Consider extracting layout calculation and handling unreasonably wide strings to avoid overflow/panic in rendering math.Mutex::lock().unwrap(); any poisoning (e.g., panic during signal handling) will panic again. Signal handlers should avoid unwraps and degrade gracefully to prevent double panics during shutdown.unwrap()s on channel send/recv in the bridge; if the channel is closed (e.g., runtime shutdown), the bridge will panic instead of allowing a clean exit. Return errors and let the main loop terminate gracefully.unwrap() around serialization and system resource reads; a failure to read system memory or serialize limits will panic instead of bubbling a configuration error. Prefer error propagation.RwLock::unwrap() and channel unwrap()s. A poisoned lock or closed channel will panic plugins and potentially the editor. Replace with error paths that surface to the plugin caller or log-and-drop to keep the host stable.RwLock::unwrap() in production paths; a poisoned lock (plugin panic) will crash the editor. Handle lock errors and surface failures to plugins.cursor.block_anchor.unwrap() when in block mode; if state enters block mode without an anchor, movement will panic. Guard against None to keep editing stable.recv().unwrap(); closed channels during shutdown will panic the host. Treat channel closure as cancellation to allow clean teardown.recv() loops have no timeout/backoff; a misbehaving plugin can hang the thread. Consider async or timeout-aware handling to avoid host hangs.apply_event_with_hooks appears to be legacy/test-only and is not part of the current editor edit path (which applies edits via Editor::apply_event_to_active_buffer and queues plugin hooks asynchronously). If this module becomes production-critical again, it should isolate hook execution (catch_unwind/log) and avoid panics on poisoned locks.InsertText, DeleteRange, InsertAtCursor) mutate buffers without going through Editor::apply_event_to_active_buffer, which can bypass cross-cutting concerns (LSP change notifications, search highlight clearing, cursor/view sync, plugin after-insert/delete hooks). Consider routing “real edit” plugin commands through the centralized apply path, or document them as “raw/unsafe” edits.render_for_split packs tab width calculation, scroll computation, and rendering into one long function; consider splitting into layout calculation and render steps to improve readability and reduce bugs in scroll math.unwrap(); production parsing should handle malformed plugin menu contributions gracefully.TempDir::new().unwrap()/read_dir().await unwraps; ensure production slow backend surfaces IO errors cleanly.unwrap() the selected item; production popup handling should guard empty lists.