# Quality Refactor Design

Date: 2026-03-06

## Goal

Address all quality issues identified in the codebase audit: critical bugs, code duplication,
Grid encapsulation, monolith decomposition, and miscellaneous improvements.

## Section 1: Critical Bugs

### 1.1 Remove `expect()` in production code

- `session.rs:271` — replace `expect` with `Entry` API or safe unwrap pattern
- `grid.rs:257` (`remove_visible_row`) — add bounds check, use `debug_assert` + safe fallback

### 1.2 Remove `catch_unwind(AssertUnwindSafe(...))`

- `session.rs:113` — remove catch_unwind. If VTE parser panics, let mutex poison.
  The reader loop already handles poisoned mutex at line 116-118.

### 1.3 Session Drop — add `debug_assert` for blocking context

- Add `fn shutdown(self)` that performs cleanup explicitly
- `Drop` impl calls `shutdown` but adds `debug_assert` warning in debug builds
  if called on a tokio runtime thread

### 1.4 Document lock ordering

- Comment at top of `session_bridge.rs`: `screen → master → pty_writer → dims`
- No type-level enforcement for now

## Section 2: Code Duplication

### 2.1 Wide-char orphan cleanup → method on Row

```rust
impl Row {
    pub fn fix_wide_char_orphan_at(&mut self, boundary: usize)
}
```

Replace duplicate code in `Grid::resize()` and `exit_alt_screen()`.

### 2.2 Resize/SIGWINCH → helper in session_bridge.rs

```rust
async fn resize_or_sigwinch(master, screen, dims, cols, rows, name) -> Result<()>
```

Replace 3 duplicated patterns (reattach resize, same-size SIGWINCH, client resize msg).

### 2.3 Server launcher — use existing constants

Replace hardcoded `0..50` and `from_millis(100)` with `SERVER_POLL_MAX` and `SERVER_POLL_INTERVAL`.

### 2.4 Skip: render cell loop and blank.clone()

Render loops differ enough that extraction hurts readability. `Cell` is 8 bytes, clone is free.

## Section 3: Grid Encapsulation

**All Grid fields become private.** New API:

### Public read-only accessors

```rust
pub fn cols(&self) -> u16
pub fn rows(&self) -> u16
pub fn cursor_pos(&self) -> (u16, u16)
pub fn cursor_visible(&self) -> bool
pub fn scroll_region(&self) -> (u16, u16)
pub fn modes(&self) -> &TerminalModes
pub fn scrollback_len(&self) -> usize
pub fn pending_start(&self) -> usize
pub fn style_table(&self) -> &StyleTable
pub fn wrap_pending(&self) -> bool
```

### pub(super) mutation methods (for performer.rs)

```rust
// Cursor with bounds checking
pub(super) fn cursor_x(&self) -> u16
pub(super) fn cursor_y(&self) -> u16
pub(super) fn set_cursor(&mut self, x: u16, y: u16)    // clamps + clears wrap
pub(super) fn set_cursor_x(&mut self, x: u16)           // clamps to cols-1
pub(super) fn set_cursor_y(&mut self, y: u16)           // clamps to rows-1

// Scroll region
pub(super) fn set_scroll_region(&mut self, top: u16, bottom: u16)

// Modes
pub(super) fn modes_mut(&mut self) -> &mut TerminalModes

// Tab stops
pub(super) fn set_tab_stop(&mut self, col: u16)
pub(super) fn clear_tab_stop(&mut self, col: u16)
pub(super) fn clear_all_tab_stops(&mut self)
pub(super) fn reset_tab_stops(&mut self)

// Style
pub(super) fn style_table_mut(&mut self) -> &mut StyleTable

// Wrap
pub(super) fn set_wrap_pending(&mut self, val: bool)

// Cursor visibility
pub(super) fn set_cursor_visible(&mut self, visible: bool)

// Scrollback
pub(super) fn set_scrollback_limit(&mut self, limit: usize)
pub(super) fn scrollback_limit(&self) -> usize
pub(super) fn set_pending_start(&mut self, val: usize)

// Alt screen (move logic from performer)
pub(super) fn enter_alt_screen(&mut self) -> SavedGrid
pub(super) fn exit_alt_screen(&mut self, saved: SavedGrid)

// Scrollback access for mod.rs
pub fn scrollback_rows(&self) -> impl Iterator<Item = &Row>
pub fn pending_scrollback_rows(&self) -> impl Iterator<Item = &Row>
```

## Section 4: Monolith Decomposition

### 4.1 `setup_session` → 3 functions

- `acquire_session()` — match on ConnectMode, return `(&mut Session, bool)`
- `resize_on_reattach()` — resize/SIGWINCH logic (uses helper from 2.2)
- `setup_session()` — orchestrator

### 4.2 `run_stdin_to_socket` → extract InputFilter

```rust
struct InputFilter { carry: Vec<u8> }

enum FilterResult {
    Forward(Vec<u8>),
    Detach { before: Vec<u8> },
    FocusIn,
    NeedMore,
}

impl InputFilter {
    fn process(&mut self, raw: &[u8]) -> FilterResult
}
```

### 4.3 Skip: `persistent_reader_loop`

Linear loop, extraction would scatter related logic.

## Section 5: Miscellaneous

### 5.1 pre_exec fd loop

Cap at `min(max_fd, 4096)` with comment explaining the trade-off.

### 5.2 Magic numbers → named constants

- `MAX_COMBINING = 16` — module level
- `PTY_WRITER_RETRY_MS = 1`
- Add explanatory comments to `BINCODE_LINE_OVERHEAD` and `size_limit = MAX_FRAME_SIZE / 2`

### 5.3 `SessionManager::get` — `&self` instead of `&mut self`

Return `Option<&Session>` for read-only. Keep `get_mut` for mutation.

### 5.4 Not changing

- `env::set_var` in tests — needs serious test refactor
- `ConnectRequest` public fields — used in one place
- Focus-out events — functional change, not quality
- `Pty` wrapper, `ScreenState` split — cosmetic risk

## Risk Assessment

Grid encapsulation (Section 3) is the highest-risk change (~200 replacements in performer.rs).
Mitigation: existing 531 tests provide strong regression coverage. Run `cargo test` after each
section. Implement Section 3 last so other fixes aren't blocked if it regresses.

## Implementation Order

1. Section 1 (critical bugs) — smallest, highest value
2. Section 2 (dedup) — moderate size, prepares Row API for Section 3
3. Section 5 (misc) — small, independent
4. Section 4 (monoliths) — moderate, independent of Grid changes
5. Section 3 (Grid encapsulation) — largest, depends on 2.1 for Row methods
