Skip to content

Conversation

@ammario
Copy link
Member

@ammario ammario commented Oct 7, 2025

Summary

  • add a dedicated VimTextArea with insert and normal modes, core motions (h/j/k/l, 0, $, w, b) and edits (x, dd, yy, p/P, u, Ctrl-r)
  • replace the raw textarea in ChatInput with the new component while preserving existing send/newline/cancel interactions
  • ensure ESC still cancels edits or interrupts streams while normal-mode ESC remains available; suppress Vim keys when slash suggestions are open

Context

  • Original user request: "Help me implement Vim support in the ChatInput box. Put it in its own component. Support the basic keybinds and modes -- like an MVP."
  • This PR fulfills that MVP by isolating Vim behavior in one reusable component and wiring it into the existing chat UI without touching backend or IPC layers.

Testing

  • Not run (local lint/typecheck currently fail because the workspace is missing required dependencies)

Generated with cmux

@ammario ammario force-pushed the josh branch 2 times, most recently from 201eecf to 9672eea Compare October 7, 2025 03:28
@chatgpt-codex-connector

This comment was marked as resolved.

@ammario ammario force-pushed the josh branch 7 times, most recently from 295d676 to e834452 Compare October 7, 2025 20:52
ammario and others added 20 commits October 8, 2025 20:36
- New VimTextArea component with basic Vim modes (insert/normal)
- Supports h/j/k/l, 0, $, w, b; i/a/I/A/o/O; x, dd, yy, p/P; u, Ctrl-r
- Integrated with ChatInput, preserves existing send/cancel keybinds
- ESC now only intercepted for edit/interrupt; otherwise passes to Vim or suggestions
- Avoids interfering with CommandSuggestions via suppressKeys

_Generated with `cmux`_
…dicator. Simulate block cursor in normal mode via 1-char selection + caret-color: transparent.
… tests

- Create src/utils/vim.ts with pure functions for all Vim operations
- 43 unit tests covering all motions, edits, and change operators
- Refactor VimTextArea to use vim utils, removing 200+ lines of duplication
- All operations now testable without component overhead
- Fix unused variable lint errors in vim.ts
- Fix unsafe type access issues in VimTextArea
- Remove deprecation warnings (execCommand is supported in Chromium)
- Move VIM_MODE_SUMMARY.md to src/components/VimTextArea.md (colocated)
- Add documentation organization guidelines to AGENTS.md:
  - No free-floating markdown docs
  - User docs → ./docs/
  - Developer docs → colocated with code
- Increase cursor opacity from 0.3 to 0.6 for better visibility
- Fix cursor not showing at end of word by checking p < value.length instead of p < lineEnd
- Reduce spacing by using min-height: 11px instead of height: 12px for mode indicator
When using w to move forward on the last word, the cursor was ending up
at text.length (past the end), making it invisible. In Vim normal mode,
the cursor should never go past the last character.

Now moveWordForward and moveWordBackward clamp their return value to
max(0, text.length - 1) to ensure cursor stays on the last character.
Changes:
- Add KEYBINDS.CANCEL_EDIT (Ctrl+Q) for canceling message edits
- Separate edit cancel from ESC/CANCEL keybind
- ESC now only handles: Vim mode transitions and stream interruption
- Update editing indicator to show "Ctrl+Q to cancel" (or Cmd+Q on macOS)
- Update keybinds documentation

This allows Vim mode to work properly when editing messages - ESC switches
to normal mode as expected, while Ctrl+Q cancels the edit.
ESC was incorrectly trying to interrupt streams. Stream interruption is
properly handled by Ctrl+C (INTERRUPT_STREAM keybind) in AIView.tsx.

Now ESC properly passes through to VimTextArea for Vim mode transitions.
… support

Major refactor to make Vim implementation elegant and composable:

**New Composable Architecture:**
- Unified applyOperator() function handles all operator-motion combinations
- Operators: d (delete), c (change), y (yank)
- Motions: w, b, $, 0, plus doubled operator for line (dd, yy, cc)
- Text objects: iw (inner word)

**New Commands:**
- d$ (or D) - delete to end of line
- d0 - delete to beginning of line
- dw, db - delete with word motions
- y$, y0, yw, yb - yank with all motions
- All c motions already worked, now share same code path

**Design Benefits:**
- Adding a new motion automatically makes it work with ALL operators
- No more duplicate code for each operator-motion combination
- Easy to extend: just add motion to type and handle once
- ~150 lines added but eliminated 57 lines of duplication

**Removed:**
- Unused deleteLine() and yankLine() helper functions
- Legacy separate 'c' operator handling (now uses unified system)
Changes:
- Remove cursor blinking - now solid block like real Vim
- Remove cursorVisible state and blink interval
- Simplified cursor logic - always show selection in normal mode
- Add EmptyCursor overlay for when textarea is empty
  - Shows a solid block at start position
  - Only visible in normal mode with no text
  - Matches Vim behavior of showing cursor on empty buffer

The cursor is now always visible in normal mode, never blinks, and shows
even when there's no text - just like in real Vim!
In Vim, the $ motion moves to the LAST character of the line, not the
position after it. This fixes the cursor disappearing when pressing $.

Before: lineEnd pointed past the last char, cursor had nothing to select
After: lineEnd - 1 points to the last char, cursor shows properly

Now both standalone $ and operator+$ (d$, c$, y$) work correctly with
visible cursor.
When pressing ESC from insert mode, the cursor could be positioned after
the last character (valid in insert mode). In normal mode, the cursor must
always be ON a character, so we now clamp it to value.length - 1.

This fixes d$ and c$ not working - they were operating on empty range
because cursor was already past the end.

Why didn't tests catch it?
- Tests only verify utility functions (getLineBounds returns correct value)
- Tests don't verify component behavior (how cursor should move in UI)
- Need integration/E2E tests for full user workflows
Replace isolated utility function tests with integration tests that verify
complete Vim command workflows. This approach catches bugs that unit tests
missed, such as:
- Cursor positioning across mode transitions
- Operator-motion composition
- State management between key presses

Test architecture:
- VimState interface: text, cursor, mode, yankBuffer, desiredColumn
- executeVimCommands(): simulates key press sequences
- applyOperatorMotion(): handles operator+motion combinations
- 34 tests covering: modes, navigation, edits, operators, workflows, edge cases

Benefits over previous approach:
- Tests user-facing behavior, not implementation details
- Catches integration bugs (e.g., $ motion cursor visibility, d$ not working)
- Self-documenting - shows how commands actually work
- Easier to add new test cases

All 34 tests passing. No new TypeScript errors.
Add support for text objects (iw - inner word) in the test harness.
Previously, 'ciw' was incorrectly treated as 'cw' because 'w' was matched
as a motion before checking for text objects.

Fix: Only check for motions if no text object was already set.

Changes:
- Add pendingTextObj state for two-key text object sequences
- Implement applyOperatorTextObject() for text object operations
- Guard motion detection with !textObject check
- Add tests for both reported issues

Tests:
- issue #1: ciw correctly deletes inner word (test passes)
- issue #2: o on last line inserts newline (already worked)

All 36 tests passing.
Bug: After ciw (change inner word), a blank character remained selected,
causing the next typed character to replace it.

Root cause: In applyEditAndEnterInsert(), setCursor() was called while
still in normal mode. setCursor() checks vimMode and creates a block
cursor selection (p+1) when in normal mode. This selection persisted
after switching to insert mode.

Fix: Call setVimMode("insert") BEFORE setCursor() so that setCursor()
sees insert mode and creates a thin cursor (no selection).

This affects all change operations that enter insert mode:
- ciw, cw, cb, c$, c0, cc, C

All 36 tests passing.
ammar-agent and others added 9 commits October 8, 2025 20:37
Improvements:
1. Mode indicator now shows pending operator (e.g., "NORMAL d", "NORMAL ci")
   - Shows operator (d/c/y) and accumulated args (like "i" in "ciw")
   - Updates dynamically as user types multi-key commands

2. Fixed border bump when NORMAL appears
   - Changed min-height to fixed height: 11px
   - Added line-height: 11px for vertical centering
   - Border no longer shifts when mode indicator shows/hides

3. Removed excessive spacing
   - Removed margin-bottom: 2px from ModeIndicator
   - Tighter integration with textarea

UX now matches Vim's command display more closely.
Improvements:
1. Reduced padding for tighter layout
   - Textarea padding: 8px 12px → 6px 8px
   - Min-height: 36px → 32px
   - Added 1px margin-bottom to mode indicator for minimal spacing

2. Better debugging in dev tools
   - Added data-component attributes to container divs
   - data-component="VimTextAreaContainer" on outer wrapper
   - data-component="VimTextAreaWrapper" on relative positioned wrapper
   - Easier to identify elements in inspect tools

3. Adjusted EmptyCursor positioning to match new padding
   - left: 12px → 8px
   - top: 8px → 6px

Layout is now more compact with less wasted space.
Improvements:
1. Added HelpIndicator with tooltip
   - Uses existing TooltipWrapper/HelpIndicator pattern
   - Positioned to the LEFT of mode text
   - Links to comprehensive Vim docs

2. Fixed uppercase issue
   - Moved text-transform: uppercase to ModeText component
   - Mode name "normal" is uppercase: "NORMAL"
   - Pending commands stay lowercase: "d", "ci", etc.
   - Now shows: "? NORMAL d" instead of "? NORMAL D"

3. Created comprehensive Vim documentation
   - docs/vim-mode.md with full command reference
   - Covers all implemented features
   - Navigation, editing, operators, motions, text objects
   - Tips, keybind conflicts, architecture notes
   - Lists unimplemented features for future

4. Improved layout
   - Mode indicator uses flexbox with gap: 4px
   - Help indicator, mode name, and pending command aligned

Visual result: ? NORMAL d (with hoverable ? for help)

All 36 tests passing.
Added guidance to Documentation Guidelines in AGENTS.md:
- Emphasize reading docs/README.md first
- Explain mdbook structure and deployment
- Note that docs must be in SUMMARY.md to appear
- Remind about mermaid diagram support

This ensures agents understand the mdbook workflow before creating
user-facing documentation.

Note: docs/vim-mode.md was created before mdbook structure exists.
Once mdbook is set up, it should be moved to docs/src/ and added
to SUMMARY.md.
Fixes:
1. AGENTS.md: docs/src/SUMMARY.md → docs/SUMMARY.md
   - The mdbook src is "." (root of docs/), not a src/ subdirectory
   - This was outdated information

2. Added vim-mode.md to docs/SUMMARY.md
   - Now appears in the table of contents after Keyboard Shortcuts
   - Will be visible on https://cmux.io once deployed

The mdbook structure IS set up and working!
Improvements:
1. Reduced excessive top padding
   - InputSection: padding: 15px → 5px 15px 15px 15px
   - Top padding reduced from 15px to 5px for tighter layout
   - Reduces wasted space above textarea

2. Added data-component labels for debugging
   - data-component="ChatInputSection" (outer container)
   - data-component="ChatInputControls" (contains VimTextArea)
   - data-component="ChatModeToggles" (mode buttons area)

Component tree for debugging:
ChatInputSection
  └─ ChatInputControls
      └─ VimTextAreaContainer
          └─ VimTextAreaWrapper
              └─ textarea.VimTextArea

All 36 tests passing.
…user docs

- Add HTML comment at top of docs/vim-mode.md noting sync requirements
- Add sync comments to VimTextArea.tsx, vim.ts, and vim.test.ts
- Remove 'Architecture Notes' section from user-facing documentation
  (implementation details don't belong in user docs)

_Generated with `cmux`_
docs/README.md:
- Add 'Writing Guidelines' section emphasizing focus on what matters
- Document what NOT to document (expected behavior, obvious details)
- Document what TO document (deviations, complex workflows, core concepts)
- Provide concrete examples of both

docs/vim-mode.md:
- Condense Visual Feedback section (remove trivial cursor details)
- Remove 'Other Keybinds' section (obvious deferral behavior)
- Remove 'Visual feedback' tip (redundant with Visual Feedback section)
- Consolidate Tips from 4 to 3 items

Principle: Users expect standard Vim behavior. Only document what's
different, complex, or non-obvious. Avoid documenting trivia.

_Generated with `cmux`_
- Add VimState.mode field and VimKeyResult type
- Implement handleKeyPress() as single entry point for all Vim key handling
- Move operator state machine logic from component to vim.ts
- Move mode transitions, navigation, edits, operators all to vim.ts
- Fix paste behavior: 'p' pastes AFTER cursor character (not at cursor)
- Fix '$' on empty lines: stays at lineStart instead of lineStart-1

Test refactor:
- Replace 350-line test harness with 25-line wrapper around handleKeyPress()
- All 36 tests pass using real implementation
- Tests now validate actual state machine, not test-specific simulation

Benefits:
- State machine is 100% testable without React/DOM
- All Vim logic in one place, fully tested
- Component will become much simpler (just applies state updates)
- No more timing hacks or scattered state management

_Generated with `cmux`_
BEFORE: 634 lines with scattered logic
AFTER: 263 lines of clean React glue code

Changes:
- Remove 422 lines of duplicated Vim logic (moved to vim.ts)
- Replace complex key handling with single handleKeyPress() call
- Change pendingOp from useRef to useState for clean state flow
- Delete all helper functions: moveVert, applyEdit, applyOperator, etc.
- New handleKeyDownInternal: 61 lines vs old 400+ lines

Component now just:
- Builds VimState from React state
- Calls vim.handleKeyPress()
- Applies result to React state
- Handles undo/redo side effects

Benefits:
- 371 fewer lines to maintain
- All logic tested in vim.ts (36/36 tests pass)
- No code duplication
- Clear separation: vim.ts = logic, component = React glue
- Eliminates 95-line applyOperator() function
- Eliminates 250+ line handleNormalKey() function

All tests pass, TypeScript happy, no behavioral changes.

_Generated with `cmux`_
Implementation:
- Add moveWordEnd() function in vim.ts
- Wire up in tryHandleNavigation() for e/E keys
- Add to handlePendingOperator() for operator+motion (de, ce, ye)
- Add cases in applyOperatorMotion() for all three operators (d/c/y)

Tests:
- Add 3 new tests for e motion (39 total, all passing)
- Test navigation: e moves to end of current word
- Test operators: de, ce work correctly

Docs:
- Update vim-mode.md with e/E descriptions
- Add to Motions list and Examples
- Move ge to "Not Yet Implemented" (only backward end not done yet)

Result:
- Users can now use e/E for end-of-word navigation
- Works with all operators: de, ce, ye, etc.
- Feels snappier! 🚀

_Generated with `cmux`_
Extract motion-to-range calculation into getMotionRange() helper, reducing
applyOperatorMotion from ~120 lines to ~80 lines by eliminating three
near-identical switch statements (one per operator: d, c, y).

Remove now-unused wrapper functions:
- changeWord, changeToEndOfLine, changeToBeginningOfLine, changeInnerWord
These were thin wrappers around changeRange() that are no longer needed.

Move mode indicator formatting logic from VimTextArea.tsx to vim.ts via
new formatPendingCommand() helper, keeping display logic with state logic.

Impact:
- vim.ts: 943 → 897 lines (-46 lines)
- VimTextArea.tsx: 263 → 257 lines (-6 lines)
- All 39 tests passing
- No functional changes, pure refactoring

_Generated with `cmux`_
Add proper type guard for insert mode keys (isInsertKey) to eliminate
'as any' casts and provide compile-time type safety.

Extract completeOperation() helper to reduce boilerplate when returning
new state after operations. Every operator completion had this pattern:
  {
    ...state,
    text: result.text,
    cursor: result.cursor,
    yankBuffer: result.yankBuffer,
    pendingOp: null,
    desiredColumn: null,
  }

Now simplified to:
  completeOperation(state, {
    text: result.text,
    cursor: result.cursor,
    yankBuffer: result.yankBuffer,
  })

The helper automatically clears pendingOp and desiredColumn.

Impact:
- Eliminated all 'as any' type assertions
- Reduced 12 state return blocks from 7 lines each to 4 lines
- vim.ts: 897 → 892 lines (net -5, but gained reusability)
- All 39 tests passing

_Generated with `cmux`_
…plate

Add handleKey() helper to wrap the common pattern of creating handled
key results with updated state. This pattern appeared 30+ times:
  {
    handled: true,
    newState: { ...state, cursor: newCursor, desiredColumn: null }
  }

Now simplified to:
  handleKey(state, { cursor: newCursor, desiredColumn: null })

Applied throughout:
- tryHandleNavigation(): All 10 navigation cases (h/j/k/l/w/b/e/0/$)
- tryHandleEdit(): All edit commands (x/p/P)
- tryEnterInsertMode(): Insert mode entry (i/a/I/A/o/O)
- tryHandleOperator(): Operator commands (d/c/y)
- handleInsertModeKey(): ESC to normal mode
- handlePendingOperator(): Operator+motion combinations

Benefits:
- Reduced 30+ multi-line returns to single lines
- Less noise, more focus on business logic
- Consistent pattern throughout codebase

Impact:
- vim.ts: 892 → 811 lines (-81 lines, -9%)
- All 39 tests passing
- No functional changes

_Generated with `cmux`_
Make the '?' help indicator next to NORMAL mode slightly smaller for
better visual balance:
- Font size: 8px → 7px
- Circle size: 11px × 11px → 10px × 10px
- Line height: 9px → 8px

_Generated with `cmux`_
The setCursor function was checking vimMode from closure, which would be
stale when called in setTimeout after a mode transition (like ciw entering
insert mode). This caused it to think we were still in normal mode and
create a 1-char selection (highlight).

Solution: Pass the new mode explicitly to setCursor to avoid stale closure.

Example bug:
- Text: "hello world foo" with cursor on "world"
- Execute: ciw
- Expected: cursor at position 6, no highlight (insert mode)
- Before: cursor highlighted next character (stale normal mode check)
- After: cursor positioned correctly without highlight

_Generated with `cmux`_
- Use nullish coalescing (??) instead of logical OR (||) in formatPendingCommand
- Add type assertions for VERSION module properties to satisfy type safety

These were blocking CI checks.

_Generated with `cmux`_
@ammario ammario enabled auto-merge October 9, 2025 04:01
@ammario ammario added this pull request to the merge queue Oct 9, 2025
Merged via the queue into main with commit d93530e Oct 9, 2025
7 of 8 checks passed
@ammario ammario deleted the josh branch October 9, 2025 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants