Skip to content

fix(editor): stop the rectForOffset crash and keep the autocomplete popup below the caret#1835

Merged
datlechin merged 1 commit into
mainfrom
fix/editor-rectForOffset-index-crash
Jul 8, 2026
Merged

fix(editor): stop the rectForOffset crash and keep the autocomplete popup below the caret#1835
datlechin merged 1 commit into
mainfrom
fix/editor-rectForOffset-index-crash

Conversation

@datlechin

Copy link
Copy Markdown
Member

Two related editor fixes, both surfaced while testing autocomplete on macOS 26.

1. Crash while typing (and the autocomplete popup going missing)

Typing in the SQL editor could quit the app with NSInvalidArgumentException: The index N is invalid, and the autocomplete popup often failed to appear.

Root cause: TextLayoutManager.rectForOffset(_:) returns CGRect? (meant to be safe) but guarded only offset < lineStorage.length, then called rangeOfComposedCharacterSequence(at: offset) on the text storage with no guard against the string's length. The editor's minimap keeps its own layout manager whose line storage is transiently longer than the shared string during an edit, so an offset valid for the line storage but past the string end made that call raise. From the selection path it spammed the log; from the draw path (TextView.draw -> drawSelections -> getFillRects -> rectForOffset) it raised inside drawRect and AppKit aborted. The same exception, thrown from SelectionManager.didReplaceCharacters during a keystroke, unwound the edit before the didReplaceContentsIn delegate that opens the autocomplete popup could run, which is why the popup went missing.

Fix: guard the rangeOfComposedCharacterSequence calls in rectForOffset and the sibling rectsFor(range:in:) against the text length, so both honor their safe contracts for every caller. The edit then completes, so the popup trigger fires.

Regression test reproduces the desync deterministically (a standalone TextLayoutManager whose text storage is shrunk without notifying it), and the sibling range path is exercised too.

2. Autocomplete popup covering the current line / landing off in the corner

The completion popup was constrained vertically to the editor pane, so a tall popup with the caret near the top of a short editor flipped above the caret and covered the line you were typing. An intermediate attempt to constrain it to the screen made it worse: the flip-above clamp pinned it to the screen top, far from the caret.

Fix: the panel is always anchored to the caret. It drops below the caret (top edge at the caret's baseline) so the current line stays visible, flips above only when the caret is near the screen bottom, and is never pinned to a screen edge. A tall panel clips at the screen edge it runs into. The horizontal editor constraint is kept so the panel still doesn't cover the right-side toolbar. This matches how VS Code positions its completion widget; clicks over any overflow dismiss it (shipped in #1831).

Pure placement math with unit tests, including a regression test for the "caret low on screen, don't fling to the top" case.

Notes

  • All in the vendored LocalPackages/CodeEditTextView and LocalPackages/CodeEditSourceEditor packages (no PluginKit ABI impact).
  • Verified with swift test on both packages: rectForOffset and placement suites pass. The app build is not run here; please build to confirm.
  • The minimap's own "suspend work while hidden" behavior already lives on main, so this PR does not touch it.

@datlechin datlechin force-pushed the fix/editor-rectForOffset-index-crash branch from 82dd109 to 17f5743 Compare July 8, 2026 15:40
@datlechin datlechin merged commit 5790b46 into main Jul 8, 2026
@datlechin datlechin deleted the fix/editor-rectForOffset-index-crash branch July 8, 2026 15:42

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 82dd109160

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// Guard against a range past the text storage's length: the line storage can be transiently longer than the
// string during an edit, and `rangeOfComposedCharacterSequence` raises `NSInvalidArgumentException` for an
// out-of-bounds index.
guard range.length > 0, range.upperBound <= textStorage.length else { return [] }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clamp only the stale tail of rect ranges

When the line storage is transiently longer than the string, any selection/highlight range that starts in still-valid text but extends into the stale tail now returns no rects at all because this guard rejects the whole range. In that desync scenario, callers such as selection/emphasis drawing lose the visible highlight for the valid prefix instead of just avoiding the out-of-bounds rangeOfComposedCharacterSequence call; clamp the range to textStorage.length before computing composed-character bounds.

Useful? React with 👍 / 👎.

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.

1 participant