fix: update find searchScope when the selection changes - #331038
Open
aidaodedjl wants to merge 2 commits into
Open
fix: update find searchScope when the selection changes#331038aidaodedjl wants to merge 2 commits into
aidaodedjl wants to merge 2 commits into
Conversation
The searchScope change detection in FindReplaceState.change() had inverted comparison logic since the multi-selection refactor (b9efdab): a new scope was treated as unchanged whenever any existing scope range differed from it, so moving or resizing the selection kept the stale scope and the match count never updated. Compare scopes with a proper set-equality check instead. Fixes microsoft#237774
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds regression coverage for search-scope updates in FindController and fixes searchScope change detection so identical scopes don’t emit state-change events.
Changes:
- Added a new test covering
searchScopeupdates and “no event on identical scope”. - Introduced
searchScopesEqualhelper and replaced the previous scope-change comparison logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/editor/contrib/find/test/browser/findController.test.ts | Adds regression test for searchScope updates and event emission behavior. |
| src/vs/editor/contrib/find/browser/findState.ts | Adds searchScopesEqual helper and uses it to avoid emitting changes for equivalent scopes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (!a || !b || a.length !== b.length) { | ||
| return false; | ||
| } | ||
| return a.every(rangeA => b.some(rangeB => Range.equalsRange(rangeA, rangeB))); |
Comment on lines
+681
to
+686
| test('issue #237774: Update searchScope when the selection changes', async () => { | ||
| await withAsyncTestCodeEditor([ | ||
| 'var x = (3 * 5)', | ||
| 'var y = (3 * 5)', | ||
| 'var z = (3 * 5)', | ||
| ], { serviceCollection: serviceCollection }, async (editor) => { |
| findState.change({ searchScope: [new Selection(1, 1, 2, 1)] }, false); | ||
| listener.dispose(); | ||
| assert.strictEqual(changeEventFired, false); | ||
|
|
Author
|
@microsoft-github-policy-service agree |
Track matched indices so a multiset comparison is performed: [A, A] must not be considered equal to [A, B]. Also add a regression assertion for duplicate ranges and rename the test to reflect that it exercises scope changes directly.
Author
|
Addressed all three review comments in b88d231:
All 22 tests in |
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
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
The
searchScopechange detection inFindReplaceState.change()has inverted comparison logic since the multi-selection refactor in b9efdab (2020): a new scope was considered unchanged whenever any existing scope range differed from one of the new ranges. As a result, moving or resizing the selection while Find in Selection is active kept the stale scope, and the match count never updated.This is the root cause of the symptoms in #237774:
The fix
Replace the inverted per-range comparison with a proper set-equality check (
searchScopesEqual): same length, and every new range must be equal to some existing range. The scope is now updated whenever the selection actually changes, and no redundant change event fires when the scope is identical.Testing
issue #237774: Update searchScope when the selection changescovering: moving a single selection, shrinking from two selections to one, identical scope firing no change event, and clearing the scope.findController.test.tspass, including the existing multi-selection tests from Find/Replace using selection is very messed up #27083 and Find fails when "Auto Find in Selection" is enabled. #58604 (no regression).Fixes #237774