fix(accept): add space after every accepted word, not just the last one#697
Merged
Conversation
"Add Space After Accepting" only fired when an accept exhausted the whole suggestion. Because the model almost always returns multi-word suggestions, accepting a single word (the normal case) never exhausted it, so the toggle appeared to do nothing. Now a word-ending accept also consumes the suggestion's own following space, so the trailing space lands with this accept. Only whitespace the model already placed is taken (never synthesized), so the accepted chunk stays an exact prefix of the buffered suggestion: the session stays in lockstep with the field, the next chunk loses the leading space this one absorbed (no double space), and punctuation/newlines/space-less scripts are left untouched. The final-word case (no following space) still appends via insertionTextApplyingAutoSpace.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+677
to
+735
| func test_acceptanceChunkConsumingTrailingSpace_takesFollowingSpaceAfterWord() { | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace("world", remainingText: "world how are you"), | ||
| "world " | ||
| ) | ||
| } | ||
|
|
||
| func test_acceptanceChunkConsumingTrailingSpace_keepsLeadingWhitespaceAndTakesFollowingSpace() { | ||
| // nextAcceptanceChunk returns leading whitespace with the token, so the extension must keep it | ||
| // and still consume the space that follows the word. | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace(" world", remainingText: " world how"), | ||
| " world " | ||
| ) | ||
| } | ||
|
|
||
| func test_acceptanceChunkConsumingTrailingSpace_takesWholeHorizontalRun() { | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace("world", remainingText: "world\t how"), | ||
| "world\t " | ||
| ) | ||
| } | ||
|
|
||
| func test_acceptanceChunkConsumingTrailingSpace_noFollowingWhitespaceLeavesChunkUntouched() { | ||
| // End of the suggestion: nothing to consume here — the exhaustion-time append covers it. | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace("world", remainingText: "world"), | ||
| "world" | ||
| ) | ||
| } | ||
|
|
||
| func test_acceptanceChunkConsumingTrailingSpace_doesNotCrossNewline() { | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace("line", remainingText: "line\nnext"), | ||
| "line" | ||
| ) | ||
| } | ||
|
|
||
| func test_acceptanceChunkConsumingTrailingSpace_doesNotConsumeBeforePunctuation() { | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace("world", remainingText: "world, how"), | ||
| "world" | ||
| ) | ||
| } | ||
|
|
||
| func test_acceptanceChunkConsumingTrailingSpace_skipsWhenChunkEndsInPunctuation() { | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace("done.", remainingText: "done. next"), | ||
| "done." | ||
| ) | ||
| } | ||
|
|
||
| func test_acceptanceChunkConsumingTrailingSpace_skipsForSpacelessScript() { | ||
| // CJK scripts do not separate words with spaces, so even a stray following space is not taken. | ||
| XCTAssertEqual( | ||
| SuggestionSessionReconciler.acceptanceChunkConsumingTrailingSpace("資料", remainingText: "資料 です"), | ||
| "資料" | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
The 8 new unit tests thoroughly exercise acceptanceChunkConsumingTrailingSpace in isolation, but there is no test in SuggestionCoordinatorAcceptanceTests that exercises the full path: autoSpaceAdjustedChunk → insertionChunk → commitAcceptedChunk → presentAdvancedOverlay with addSpaceAfterAccept = true and a multi-word suggestion. If a future refactor accidentally breaks the wiring at the coordinator layer (e.g., passing the pre-adjusted chunk to commitAcceptedChunk by mistake), no test would catch it.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Owner
Author
There was a problem hiding this comment.
Good call — added two coordinator-level integration tests in 76bcae2 (test_acceptCurrentSuggestion_withAddSpaceAfterAccept_insertsTrailingSpaceOnNonFinalWord and the OFF counterpart). They drive the full path through acceptCurrentSuggestion() with a multi-word suggestion and assert the inserted chunk and remaining tail, so a coordinator-layer wiring break is caught.
…to-space Exercises the full path (autoSpaceAdjustedChunk -> insertionChunk -> commitAcceptedChunk -> state) with addSpaceAfterAccept on and off against a multi-word suggestion, so a future refactor that breaks the coordinator wiring is caught even though the pure rule is unit-tested. Addresses Greptile P2.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
"Add Space After Accepting" (#675) appeared to do nothing. The gate only fired when an accept exhausted the whole suggestion (
session.advancing(by: acceptedChunk.count).isExhausted). Because the model almost always returns multi-word suggestions, accepting a single word — the normal case — never exhausts it, so no space was ever added unless you accepted the very last word or a one-word suggestion.Now a word-ending accept also consumes the suggestion's own following space, so the trailing space lands with this accept on every word. This is done by extending the accepted chunk, not by synthesizing a character mid-suggestion, which is what keeps the delicate space system intact:
world, how), newlines (line\nnext), chunks already ending in punctuation (done.), and space-less scripts (CJK) are left untouched — the model's own layout wins there.Net: accept "world" from "world how are you" → inserts
worldand the ghost continues withhow are you; keep accepting and you getworld how are you, or stop and type with the caret already spaced.Validation
Added 8 unit tests for the new
acceptanceChunkConsumingTrailingSpacerule (following space, leading-whitespace chunks, whole horizontal runs, end-of-suggestion no-op, newline boundary, pre-punctuation no-op, chunk-ends-in-punctuation, space-less script).Linked issues
Risk / rollout notes
autoSpaceAdjustedChunkreturns the chunk unchanged — byte-for-byte prior behavior.lastAcceptedTail) handles that consistently because it is still a literal prefix of the suggestion tail.Greptile Summary
This PR fixes a bug where the "Add Space After Accepting" setting only fired when accepting the very last word of a suggestion (exhausting it), not on every mid-suggestion word accept. The fix moves the space-append logic from the insertion-text layer (synthesized, post-session) to the chunk layer (consuming the model's own following space as an exact prefix of the remaining text), so the session stays in lockstep with the live field on every accept.
autoSpaceAdjustedChunk(new) extends the accepted chunk to include the suggestion's own trailing horizontal whitespace before the full insertion/session-advance/overlay pipeline runs, whileinsertionTextApplyingAutoSpace(existing, unchanged) still handles the exhausting-accept case where no following suggestion space exists to consume.acceptanceChunkConsumingTrailingSpacecover following-space, multi-space runs, newline boundary, pre-punctuation, chunk-ends-in-punctuation, space-less script, and end-of-suggestion; two new coordinator-level integration tests verify the full path with the setting on/off.Confidence Score: 5/5
Safe to merge — the change is gated behind the off-by-default 'add space after accepting' toggle, the new chunk-extension path is a pure function of data the session already holds, and the double-space interaction between the two spacing mechanisms is correctly precluded by existing guards.
The chunk-extension approach keeps the accepted chunk as an exact prefix of the session's remaining text, so every downstream consumer sees a consistent view. The exhaustion-time guard correctly no-ops when the extended chunk already ends in a space, preventing double-space. Both new coordinator-level integration tests and the 8 reconciler unit tests confirm the key boundary cases.
No files require special attention.
Important Files Changed
acceptanceChunkConsumingTrailingSpace, a pure function that extends a word-ending accepted chunk by the suggestion's own horizontal whitespace. Guards are well-structured; newlines are excluded from theprefix(while:)predicate.autoSpaceAdjustedChunkinto the acceptance path before the insertion chunk and auto-space exhaustion check. The existinginsertionTextApplyingAutoSpaceis unmodified and the double-space risk is precluded becauseinsertionChunkAppendingTrailingSpacegates on a non-space last character.acceptanceChunkConsumingTrailingSpacecovering all documented boundary conditions.makeCoordinatorhelper is updated to accept aSuggestionSettingsSnapshot.Sequence Diagram
sequenceDiagram participant User participant Coordinator as SuggestionCoordinator participant Reconciler as SuggestionSessionReconciler participant Session as ActiveSuggestionSession participant Inserter as SuggestionInserter User->>Coordinator: acceptCurrentSuggestion() Coordinator->>Session: prepareAcceptance(granularity: .word) Session-->>Coordinator: .ready(liveContext, session, "world") Note over Coordinator: NEW: autoSpaceAdjustedChunk Coordinator->>Reconciler: acceptanceChunkConsumingTrailingSpace("world", remainingText:"world how") Reconciler-->>Coordinator: "world " (trailing space consumed) Coordinator->>Reconciler: insertionChunk(forAcceptedChunk:"world ", precedingText:"Hello") Reconciler-->>Coordinator: "world " (no leading-space strip needed) Coordinator->>Reconciler: insertionTextApplyingAutoSpace(insertionChunk:"world ", acceptedChunk:"world ", session) Note over Reconciler: session not exhausted, returns unchanged Reconciler-->>Coordinator: "world " Coordinator->>Inserter: insert("world ") Inserter-->>Coordinator: true Coordinator->>Session: commitAcceptedChunk("world ") Session-->>Coordinator: .advanced(remainingText:"how") Coordinator->>Coordinator: "state = .ready(text:"how")" Coordinator->>Coordinator: presentAdvancedOverlay(remainingText:"how")Reviews (2): Last reviewed commit: "test(accept): add coordinator-level inte..." | Re-trigger Greptile