Adjust start offset calculation for query selection #27
Conversation
WalkthroughThe change updates acceptAutocompleteOption in multi-trigger autocomplete: when not keeping the trigger, the replacement start index now subtracts the actual trigger text length (instead of a fixed 1), altering the replaced range and final cursor position. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant Autocomplete
Note right of Autocomplete `#D6EAF8`: acceptAutocompleteOption now uses trigger.text.length
User->>Editor: Choose suggestion
Editor->>Autocomplete: acceptAutocompleteOption(suggestion, keepTrigger=false)
Autocomplete->>Autocomplete: startIndex = triggerStart - trigger.text.length
Autocomplete->>Editor: replace range(startIndex..end) with suggestion
Editor->>User: document updated, cursor repositioned
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| var start = querySelection.baseOffset; | ||
| if (!keepTrigger) start -= 1; | ||
| if (!keepTrigger) start -= _currentTrigger.trigger.length; |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Correctly fixes multi-character trigger handling.
The change properly accounts for triggers longer than one character. The previous hardcoded start -= 1 only worked for single-character triggers. Using _currentTrigger.trigger.length ensures the entire trigger is included in the replacement range when keepTrigger is false.
This affects both:
- The text replacement at line 242:
text.replaceRange(start, end, option) - The cursor position calculation at line 237:
selectionOffset = start + option.length
Consider verifying that tests cover multi-character triggers (e.g., "@@", "###") to prevent regression. You can check if test files exist and validate coverage:
🏁 Script executed:
#!/bin/bash
# Find test files and search for multi-character trigger test cases
fd -e dart -t f test | xargs rg -l "acceptAutocompleteOption|keepTrigger" | head -5
# Check for test cases with multi-character triggers
rg -A 10 "keepTrigger.*false|multi.*character.*trigger" --type dart -g '*test*.dart'Length of output: 41
🏁 Script executed:
#!/bin/bash
# Read the test file to examine test coverage
cat test/multi_trigger_autocomplete_test.dart | head -200
# Search for test groups or test functions related to acceptAutocompleteOption
rg -n "test|group|acceptAutocompleteOption" test/multi_trigger_autocomplete_test.dart | head -30Length of output: 8267
🏁 Script executed:
#!/bin/bash
# Search for keepTrigger in test file
echo "=== Searching for keepTrigger usage ==="
rg -n "keepTrigger" test/multi_trigger_autocomplete_test.dart
# Get file length and look for multi-character trigger patterns
echo "=== File statistics and multi-character trigger patterns ==="
wc -l test/multi_trigger_autocomplete_test.dart
rg -n "trigger.*[:=].*['\"][@#]{2,}" test/multi_trigger_autocomplete_test.dart
# List all test functions/groups
echo "=== All test functions in file ==="
rg -n "testWidgets|test\(" test/multi_trigger_autocomplete_test.dart | wc -l
rg -n "testWidgets\|^.*test\(" test/multi_trigger_autocomplete_test.dart | head -20Length of output: 184
Add tests for multi-character triggers with keepTrigger=false.
The fix correctly updates the trigger offset calculation, but verification shows zero test coverage for this scenario. The test file contains 10 test functions, none of which use the keepTrigger parameter or multi-character triggers.
Add tests covering:
- Multi-character triggers (e.g.,
"@@","###") withkeepTrigger=false - Correct text replacement range when entire multi-character trigger is removed
- Cursor position after autocomplete selection
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/src/multi_trigger_autocomplete.dart (1)
229-229: Bug fix correctly handles multi-character triggers.The change properly accounts for triggers longer than one character by using the actual trigger length instead of the hardcoded
1. This fixes the issue wherekeepTrigger=falsewould only remove one character of multi-character triggers like"@@"or"###".Optional: Use the local
triggervariable for consistency.Since
triggeris already null-checked at line 223, you can simplify line 229 to use the local variable directly:- if (!keepTrigger) start -= _currentTrigger?.trigger.length ?? 1; + if (!keepTrigger) start -= trigger.trigger.length;This eliminates the unnecessary null-safety operator and fallback, making the code more consistent with the pattern established by the null check above.
1 participant
Description
If trigger is longer than one character,
keepTriggerdoesn't work as expected. This fixes that :)Type of Change
Summary by CodeRabbit