-
Notifications
You must be signed in to change notification settings - Fork 7
fix: make regex opt‑in for search/replace #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a user experience issue where non-technical users experienced unexpected text replacements due to regex metacharacters being interpreted as patterns rather than literal text. The fix makes literal text matching the default behavior while providing an opt-in "Use Regular Expression" toggle for power users who need regex functionality.
Changes:
- Added
escapeRegExputility function to escape regex metacharacters for literal text matching - Introduced a "Use Regular Expression" toggle with user preference persistence via WordPress core/preferences
- Added error handling and user-friendly error messages for invalid regex patterns
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/core/utils.tsx | Added escapeRegExp function to safely escape regex metacharacters for literal string matching |
| src/core/app.tsx | Integrated regex toggle, preference persistence, error handling, and conditional escaping of search input |
| tests/utils.test.tsx | Added unit tests for the new escapeRegExp function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it( 'escapeRegExp escapes regex metacharacters', () => { | ||
| const { escapeRegExp } = require( '../src/core/utils' ); | ||
|
|
||
| const escaped = escapeRegExp( 'a.b+c*' ); | ||
|
|
||
| expect( escaped ).toBe( 'a\\.b\\+c\\*' ); | ||
| } ); |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test for escapeRegExp only covers a subset of regex metacharacters (., +, *). The implementation at line 350 escapes all regex metacharacters including ^,
| describe( 'escapeRegExp', () => { | ||
| beforeEach( () => { | ||
| jest.resetModules(); | ||
| } ); | ||
|
|
||
| it( 'escapeRegExp escapes regex metacharacters', () => { | ||
| const { escapeRegExp } = require( '../src/core/utils' ); | ||
|
|
||
| const escaped = escapeRegExp( 'a.b+c*' ); | ||
|
|
||
| expect( escaped ).toBe( 'a\\.b\\+c\\*' ); | ||
| } ); | ||
|
|
||
| it( 'escapeRegExp leaves plain text unchanged', () => { | ||
| const { escapeRegExp } = require( '../src/core/utils' ); | ||
|
|
||
| const escaped = escapeRegExp( 'plain text' ); | ||
|
|
||
| expect( escaped ).toBe( 'plain text' ); | ||
| } ); | ||
| } ); |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test coverage for escapeRegExp is missing an important edge case test that aligns with the guard clause in the implementation (line 346-348 in utils.tsx). Based on the pattern used in other tests in this file (e.g., getNumberToBase10 at lines 333-340), there should be a test case to verify that passing a non-string parameter returns an empty string.
| * @return {void} | ||
| */ | ||
| const handleUseRegex = ( newValue: boolean ): void => { | ||
| setUseRegex( newValue ); |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When toggling the regex option (line 107-110), the error state is not cleared. If a user enters invalid regex syntax while in regex mode (which shows an error), then toggles regex off, the error message will persist even though the input would now be treated as literal text and is valid. Consider clearing the error state when toggling useRegex by adding setError('') in the handleUseRegex function.
| setUseRegex( newValue ); | |
| setUseRegex( newValue ); | |
| setError( '' ); |
| describe( 'escapeRegExp', () => { | ||
| beforeEach( () => { | ||
| jest.resetModules(); | ||
| } ); | ||
|
|
||
| it( 'escapeRegExp escapes regex metacharacters', () => { | ||
| const { escapeRegExp } = require( '../src/core/utils' ); | ||
|
|
||
| const escaped = escapeRegExp( 'a.b+c*' ); | ||
|
|
||
| expect( escaped ).toBe( 'a\\.b\\+c\\*' ); | ||
| } ); | ||
|
|
||
| it( 'escapeRegExp leaves plain text unchanged', () => { | ||
| const { escapeRegExp } = require( '../src/core/utils' ); | ||
|
|
||
| const escaped = escapeRegExp( 'plain text' ); | ||
|
|
||
| expect( escaped ).toBe( 'plain text' ); | ||
| } ); | ||
| } ); |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a test case for escapeRegExp with an empty string input to verify it returns an empty string, consistent with the behavior when a non-string is passed. This would complete the edge case coverage alongside the planned test for non-string inputs.
| setError( | ||
| __( | ||
| 'Invalid regular expression.', | ||
| 'search-replace-for-block-editor' | ||
| ) |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an invalid regex error is shown and the user modifies the search input to correct it, the error will be cleared because the replace() function is called via the useEffect hook at line 121. However, if there's an error and the user clicks the "Replace" button (line 544), the error will persist if the regex is still invalid. This is correct behavior. However, consider providing more specific error information by including the actual regex error message from the caught exception, which would help users debug their regex patterns.
| setError( | |
| __( | |
| 'Invalid regular expression.', | |
| 'search-replace-for-block-editor' | |
| ) | |
| const details = | |
| err instanceof Error && err.message | |
| ? ' ' + err.message | |
| : ''; | |
| setError( | |
| __( | |
| 'Invalid regular expression.', | |
| 'search-replace-for-block-editor' | |
| ) + details |
What?
Why?
Editors reported this as a bug:
This is because
.was treated as “any character.” This is unexpected for non‑technical users.Making literal matching the default aligns with non-technical user expectations while keeping regex available.
Closes #74
.’, Replace:.".\d,\s\wto verify regex functionalityTypes of changes
Screenshots: