Adjust start offset calculation for query selection #27
Conversation
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
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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}
Comment on lines
228
to
229
| 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.
Owner
|
Hey @ViscousPot , can you add some tests for this fix? |
Comment on lines
+301
to
+324
| testWidgets('handles multi-character triggers with keepTrigger=false', (tester) async { | ||
| final multiCharTrigger = AutocompleteTrigger( | ||
| trigger: '##' | ||
| optionsViewBuilder: (context, query, controller) { }, | ||
| ); | ||
|
|
||
| await tester.pumpWidget( | ||
| Boilerplate( | ||
| child: MultiTriggerAutocomplete( | ||
| autocompleteTriggers: [multiCharTrigger], | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| await tester.enterText(find.byType(TextFormField), '##'); | ||
|
|
||
| await tester.tap(find.byType(InkWell).first); | ||
| await tester.pump(); | ||
|
|
||
| final TextFormField field = | ||
| find.byType(TextFormField).evaluate().first.widget as TextFormField; | ||
|
|
||
| expect(field.controller!.text, isNot(contains('##'))); | ||
| }); |
There was a problem hiding this comment.
Critical: Test has syntax errors and will fail at runtime.
This test has multiple issues that will cause it to fail:
- Syntax error on line 303: Missing comma after
trigger: '##' - Empty optionsViewBuilder on line 304: The function body
{ }doesn't return a Widget as required. It must return a widget like ListView with tappable items. - Line 317 will fail: Attempting to tap
find.byType(InkWell).firstbut the empty optionsViewBuilder doesn't create any InkWell widgets, causing the finder to fail. - Missing debounce wait: After line 315's
enterText, you should addawait tester.pumpAndSettle(kDebounceDuration);to allow the options to appear (see line 159 for reference). - No verification of options: Should verify
find.byType(ListView)finds a widget before attempting to tap.
Apply this diff to fix the test:
- testWidgets('handles multi-character triggers with keepTrigger=false', (tester) async {
- final multiCharTrigger = AutocompleteTrigger(
- trigger: '##'
- optionsViewBuilder: (context, query, controller) { },
- );
-
- await tester.pumpWidget(
- Boilerplate(
- child: MultiTriggerAutocomplete(
- autocompleteTriggers: [multiCharTrigger],
- ),
- ),
- );
-
- await tester.enterText(find.byType(TextFormField), '##');
-
- await tester.tap(find.byType(InkWell).first);
- await tester.pump();
-
- final TextFormField field =
- find.byType(TextFormField).evaluate().first.widget as TextFormField;
-
- expect(field.controller!.text, isNot(contains('##')));
- });
+ testWidgets('handles multi-character triggers with keepTrigger=false', (tester) async {
+ final multiCharTrigger = AutocompleteTrigger(
+ trigger: '##',
+ optionsViewBuilder: (context, query, controller) {
+ return ListView.builder(
+ itemCount: 1,
+ itemBuilder: (context, index) {
+ return ListTile(
+ title: const Text('Option'),
+ onTap: () {
+ final autocomplete = MultiTriggerAutocomplete.of(context);
+ return autocomplete.acceptAutocompleteOption('Option');
+ },
+ );
+ },
+ );
+ },
+ );
+
+ await tester.pumpWidget(
+ Boilerplate(
+ child: MultiTriggerAutocomplete(
+ debounceDuration: kDebounceDuration,
+ autocompleteTriggers: [multiCharTrigger],
+ ),
+ ),
+ );
+
+ await tester.enterText(find.byType(TextFormField), '##');
+ await tester.pumpAndSettle(kDebounceDuration);
+ expect(find.byType(ListView), findsOneWidget);
+
+ await tester.tap(find.byType(InkWell).first);
+ await tester.pump();
+
+ final TextFormField field =
+ find.byType(TextFormField).evaluate().first.widget as TextFormField;
+
+ expect(field.controller!.text, isNot(contains('##')));
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| testWidgets('handles multi-character triggers with keepTrigger=false', (tester) async { | |
| final multiCharTrigger = AutocompleteTrigger( | |
| trigger: '##' | |
| optionsViewBuilder: (context, query, controller) { }, | |
| ); | |
| await tester.pumpWidget( | |
| Boilerplate( | |
| child: MultiTriggerAutocomplete( | |
| autocompleteTriggers: [multiCharTrigger], | |
| ), | |
| ), | |
| ); | |
| await tester.enterText(find.byType(TextFormField), '##'); | |
| await tester.tap(find.byType(InkWell).first); | |
| await tester.pump(); | |
| final TextFormField field = | |
| find.byType(TextFormField).evaluate().first.widget as TextFormField; | |
| expect(field.controller!.text, isNot(contains('##'))); | |
| }); | |
| testWidgets('handles multi-character triggers with keepTrigger=false', (tester) async { | |
| final multiCharTrigger = AutocompleteTrigger( | |
| trigger: '##', | |
| optionsViewBuilder: (context, query, controller) { | |
| return ListView.builder( | |
| itemCount: 1, | |
| itemBuilder: (context, index) { | |
| return ListTile( | |
| title: const Text('Option'), | |
| onTap: () { | |
| final autocomplete = MultiTriggerAutocomplete.of(context); | |
| return autocomplete.acceptAutocompleteOption('Option'); | |
| }, | |
| ); | |
| }, | |
| ); | |
| }, | |
| ); | |
| await tester.pumpWidget( | |
| Boilerplate( | |
| child: MultiTriggerAutocomplete( | |
| debounceDuration: kDebounceDuration, | |
| autocompleteTriggers: [multiCharTrigger], | |
| ), | |
| ), | |
| ); | |
| await tester.enterText(find.byType(TextFormField), '##'); | |
| await tester.pumpAndSettle(kDebounceDuration); | |
| expect(find.byType(ListView), findsOneWidget); | |
| await tester.tap(find.byType(InkWell).first); | |
| await tester.pump(); | |
| final TextFormField field = | |
| find.byType(TextFormField).evaluate().first.widget as TextFormField; | |
| expect(field.controller!.text, isNot(contains('##'))); | |
| }); |
Comment on lines
326
to
350
| testWidgets('cursor position correct after autocomplete with multi-character trigger', (tester) async { | ||
| final multiCharTrigger = AutocompleteTrigger( | ||
| trigger: '@@', | ||
| optionsViewBuilder: (context, query, controller) { }, | ||
| ); | ||
|
|
||
| await tester.pumpWidget( | ||
| Boilerplate( | ||
| child: MultiTriggerAutocomplete( | ||
| autocompleteTriggers: [multiCharTrigger], | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| await tester.enterText(find.byType(TextFormField), '@@sa'); | ||
|
|
||
| await tester.tap(find.byType(InkWell).first); | ||
| await tester.pump(); | ||
|
|
||
| final TextFormField field = | ||
| find.byType(TextFormField).evaluate().first.widget as TextFormField; | ||
|
|
||
| expect(field.controller!.selection.baseOffset, | ||
| equals(field.controller!.text.length)); | ||
| }); |
There was a problem hiding this comment.
Critical: Test has syntax errors and will fail at runtime.
This test has the same issues as the previous test:
- Syntax error on line 328: Missing comma after
trigger: '@@' - Empty optionsViewBuilder on line 329: Must return a Widget like ListView with tappable items.
- Line 342 will fail: Attempting to tap a non-existent InkWell.
- Missing debounce wait: After line 340's
enterText, addawait tester.pumpAndSettle(kDebounceDuration);. - No verification of options: Should verify ListView is shown before tapping.
Apply this diff to fix the test:
- testWidgets('cursor position correct after autocomplete with multi-character trigger', (tester) async {
- final multiCharTrigger = AutocompleteTrigger(
- trigger: '@@',
- optionsViewBuilder: (context, query, controller) { },
- );
-
- await tester.pumpWidget(
- Boilerplate(
- child: MultiTriggerAutocomplete(
- autocompleteTriggers: [multiCharTrigger],
- ),
- ),
- );
-
- await tester.enterText(find.byType(TextFormField), '@@sa');
-
- await tester.tap(find.byType(InkWell).first);
- await tester.pump();
-
- final TextFormField field =
- find.byType(TextFormField).evaluate().first.widget as TextFormField;
-
- expect(field.controller!.selection.baseOffset,
- equals(field.controller!.text.length));
- });
+ testWidgets('cursor position correct after autocomplete with multi-character trigger', (tester) async {
+ final multiCharTrigger = AutocompleteTrigger(
+ trigger: '@@',
+ optionsViewBuilder: (context, query, controller) {
+ return ListView.builder(
+ itemCount: 1,
+ itemBuilder: (context, index) {
+ return ListTile(
+ title: const Text('Option'),
+ onTap: () {
+ final autocomplete = MultiTriggerAutocomplete.of(context);
+ return autocomplete.acceptAutocompleteOption('Option');
+ },
+ );
+ },
+ );
+ },
+ );
+
+ await tester.pumpWidget(
+ Boilerplate(
+ child: MultiTriggerAutocomplete(
+ debounceDuration: kDebounceDuration,
+ autocompleteTriggers: [multiCharTrigger],
+ ),
+ ),
+ );
+
+ await tester.enterText(find.byType(TextFormField), '@@sa');
+ await tester.pumpAndSettle(kDebounceDuration);
+ expect(find.byType(ListView), findsOneWidget);
+
+ await tester.tap(find.byType(InkWell).first);
+ await tester.pump();
+
+ final TextFormField field =
+ find.byType(TextFormField).evaluate().first.widget as TextFormField;
+
+ expect(field.controller!.selection.baseOffset,
+ equals(field.controller!.text.length));
+ });Refactor autocomplete test to include options view builder.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
If trigger is longer than one character,
keepTriggerdoesn't work as expected. This fixes that :)Type of Change
Summary by CodeRabbit
Bug Fixes
Tests