Skip to content

fix(editor): account for NFC boundary composition in insert offset - #1954

Merged
kevincodex1 merged 1 commit into
Twigpine:mainfrom
0xfandom:fix/cursor-nfc-boundary-offset
Jul 14, 2026
Merged

kevincodex1 merged 1 commit into
Twigpine:mainfrom
0xfandom:fix/cursor-nfc-boundary-offset

Conversation

@0xfandom

@0xfandom 0xfandom commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Cursor.modifyText builds newText = prefix + insert + suffix and hands it to Cursor.fromText, which NFC-normalizes the whole string. But the new cursor offset was computed as startOffset + insertString.normalize('NFC').length, normalizing the insert in isolation:

startOffset + insertString.normalize('NFC').length

When the inserted text begins with a combining mark that composes with the last character of the prefix (e.g. "e" + U+0301 → "é"), the normalized newText is one UTF-16 unit shorter than that formula assumes, so the offset overshoots by one and the cursor lands past the following text — the next keystroke edits the wrong spot.

Reachability

Cursor.insert → modifyText; insert is the mainline typed/pasted-input path in the REPL editor (useTextInput.ts, useVimInput.ts, useSearchInput.ts). A pasted fragment starting with a combining mark (or an IME delivering a lone combining mark) with the cursor right after a base letter hits it.

Fix

Measure the normalized prefix-plus-insert for the offset, so cross-boundary composition is accounted for. Reduces to the previous behavior whenever no boundary composition occurs.

Test

Cursor.insert with a combining acute between "e" and "X" → cursor at offset 1 over "éX" (was 2); plus non-composing ASCII and astral-emoji controls.

Summary by CodeRabbit

  • Bug Fixes

    • Improved cursor positioning when inserted combining characters form composed Unicode characters.
    • Corrected cursor advancement for regular and supplementary Unicode characters.
  • Tests

    • Added coverage for cursor offsets across NFC composition, standard inserts, and astral-plane characters.

Cursor.modifyText builds newText from prefix + insert + suffix and hands it to
Cursor.fromText, which NFC-normalizes the whole string. But the new cursor
offset was computed as startOffset + insertString.normalize('NFC').length,
normalizing the insert in isolation. When the inserted text begins with a
combining mark that composes with the last character of the prefix (e.g. "e" +
U+0301 -> "é"), the normalized newText is one UTF-16 unit shorter than that
formula assumes, so the returned offset overshoots by one and the cursor lands
past the following text — the next keystroke then edits the wrong spot.

Measure the normalized prefix-plus-insert instead, so cross-boundary
composition is accounted for. Reduces to the previous behavior whenever no
boundary composition occurs (plain ASCII, astral emoji, insert at start).
@coderabbitai

coderabbitai Bot commented Jul 13, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 74cf2d4c-36d0-4e8f-8015-601979bd4468

📥 Commits

Reviewing files that changed from the base of the PR and between 2448ea9 and 2b6ddb2.

📒 Files selected for processing (2)
  • src/utils/Cursor.nfc.test.ts
  • src/utils/Cursor.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: smoke-and-tests (22)
  • GitHub Check: smoke-and-tests (24.11.x)
  • GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

TypeScript code in this repository must use strict mode and ESM imports.

Files:

  • src/utils/Cursor.nfc.test.ts
  • src/utils/Cursor.ts
**

⚙️ CodeRabbit configuration file

**: # AGENTS.md - AI Agent Coding Guide

This guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.

Project Snapshot

OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.

The installed CLI runs on Node.js >=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.

Work Style

  • Keep changes focused on one problem.
  • Prefer existing patterns in the file or nearby module.
  • Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
  • Add or update tests when behavior changes.
  • Update docs when setup, commands, provider behavior, or user-facing behavior changes.
  • For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.

Stack And Conventions

  • TypeScript with strict mode and ESM imports.
  • React + Ink for terminal UI.
  • Bun lockfile and Bun scripts for development workflows.
  • Node runtime for the built CLI.

Common libraries and patterns:

  • chalk for terminal color.
  • commander for CLI argument parsing.
  • execa for child processes.
  • Existing service, provider, settings, permission, and UI patterns over new abstractions.

Repository Map

  • src/commands/ - slash and CLI command implementations.
  • src/components/ - React/Ink UI components.
  • src/services/ - API, MCP, OAuth, wiki, voice, and other service integrations.
  • src/tools/ - tool implementations.
  • src/utils/ - shared utilities.
  • src/integrations/ - provider and model integration metadata.
  • src/entrypoints/ - CLI, MCP, SDK, and generated public types.
  • src/tasks/ - local, remote, workflow, and monitor tas...

Files:

  • src/utils/Cursor.nfc.test.ts
  • src/utils/Cursor.ts
**/*

⚙️ CodeRabbit configuration file

**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.

Files:

  • src/utils/Cursor.nfc.test.ts
  • src/utils/Cursor.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}

⚙️ CodeRabbit configuration file

{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.

Files:

  • src/utils/Cursor.nfc.test.ts
🔇 Additional comments (2)
src/utils/Cursor.ts (1)

878-888: LGTM!

src/utils/Cursor.nfc.test.ts (1)

1-31: LGTM!


📝 Walkthrough

Walkthrough

Changes

Cursor NFC offset handling

Layer / File(s) Summary
NFC-aware offset calculation
src/utils/Cursor.ts
Cursor.modifyText derives the new cursor offset from the NFC-normalized prefix and inserted string.
NFC boundary regression coverage
src/utils/Cursor.nfc.test.ts
Tests cover composing combining marks, non-composing inserts, and astral-plane UTF-16 offsets.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 7
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, scoped, and accurately summarizes the NFC boundary offset fix.
Description check ✅ Passed The description covers problem, reachability, fix, and tests, so it mostly satisfies the template despite different headings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Risk Surface Disclosed ✅ Passed PR only changes Cursor offset logic and tests; it doesn’t touch auth/provider/permissions/network/CI/release surfaces, so the risk-surface check is not applicable.
No Hidden Policy Change ✅ Passed Latest commit only changes Cursor NFC offset logic and adds focused tests; no product/trust/routing/telemetry/network/permission policy code appears.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevincodex1
kevincodex1 merged commit ed3927d into Twigpine:main Jul 14, 2026
5 checks passed
hotmanxp pushed a commit to hotmanxp/openclaude that referenced this pull request Jul 15, 2026
…wigpine#1954)

Cursor.modifyText builds newText from prefix + insert + suffix and hands it to
Cursor.fromText, which NFC-normalizes the whole string. But the new cursor
offset was computed as startOffset + insertString.normalize('NFC').length,
normalizing the insert in isolation. When the inserted text begins with a
combining mark that composes with the last character of the prefix (e.g. "e" +
U+0301 -> "é"), the normalized newText is one UTF-16 unit shorter than that
formula assumes, so the returned offset overshoots by one and the cursor lands
past the following text — the next keystroke then edits the wrong spot.

Measure the normalized prefix-plus-insert instead, so cross-boundary
composition is accounted for. Reduces to the previous behavior whenever no
boundary composition occurs (plain ASCII, astral emoji, insert at start).
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.

3 participants