-
Notifications
You must be signed in to change notification settings - Fork 701
feat/add-redact-feature-on-regex-plugin #1292
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: main
Are you sure you want to change the base?
feat/add-redact-feature-on-regex-plugin #1292
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.
Added redaction feature to regex plugin with proper text processing and backward compatibility. Found several logic and performance issues that need attention.
Skipped files
plugins/default/manifest.json
: Skipped file pattern
plugins/default/regexMatch.ts
Outdated
const regex = new RegExp(regexPattern, redact ? 'g' : ''); | ||
|
||
// Check for matches across all text | ||
let hasMatches = false; | ||
const mappedTextArray: Array<string | null> = []; | ||
|
||
textArray.forEach((text) => { | ||
if (!text) { | ||
mappedTextArray.push(null); | ||
return; | ||
} | ||
|
||
// Check if pattern exists in text | ||
const localRegex = new RegExp(regexPattern, 'g'); | ||
const matches = text.match(localRegex); |
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.
🐛 Bug Fix
Issue: Creating redundant RegExp objects - regex
is created but never used, while localRegex
is recreated in every loop iteration
Fix: Reuse the main regex object and avoid unnecessary object creation
Impact: Reduces memory allocation and improves performance in loops
const regex = new RegExp(regexPattern, redact ? 'g' : ''); | |
// Check for matches across all text | |
let hasMatches = false; | |
const mappedTextArray: Array<string | null> = []; | |
textArray.forEach((text) => { | |
if (!text) { | |
mappedTextArray.push(null); | |
return; | |
} | |
// Check if pattern exists in text | |
const localRegex = new RegExp(regexPattern, 'g'); | |
const matches = text.match(localRegex); | |
const regex = new RegExp(regexPattern, redact ? 'g' : ''); | |
// Check for matches across all text | |
let hasMatches = false; | |
const mappedTextArray: Array<string | null> = []; | |
textArray.forEach((text) => { | |
if (!text) { | |
mappedTextArray.push(null); | |
return; | |
} | |
// Check if pattern exists in text | |
const matches = text.match(regex); |
plugins/default/regexMatch.ts
Outdated
if (redact) { | ||
const redactedText = text.replace(localRegex, redactText); | ||
mappedTextArray.push(redactedText); |
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.
🐛 Bug Fix
Issue: Using localRegex
variable that no longer exists after the previous fix
Fix: Use the main regex
object for replacement
Impact: Fixes reference error and maintains consistent regex behavior
if (redact) { | |
const redactedText = text.replace(localRegex, redactText); | |
mappedTextArray.push(redactedText); | |
if (redact) { | |
const redactedText = text.replace(regex, redactText); | |
mappedTextArray.push(redactedText); | |
} else { |
transformed = true; | ||
} | ||
|
||
verdict = not ? hasMatches : !shouldBlock; |
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.
🐛 Bug Fix
Issue: Logic error in verdict calculation - when not=true
and hasMatches=true
, should return false
(blocked), but !shouldBlock
returns true
Fix: Correct the boolean logic for the not
parameter
Impact: Fixes inverted logic that would allow matches when they should be blocked
verdict = not ? hasMatches : !shouldBlock; | |
verdict = not ? !hasMatches : !shouldBlock; |
plugins/default/regexMatch.ts
Outdated
// For backward compatibility, also get single text for matchDetails | ||
const textToMatch = getText(context, eventType); | ||
const singleMatch = new RegExp(regexPattern).exec(textToMatch); |
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.
⚡️ Performance Improvement
Issue: Creating new RegExp object for single match when regex already exists
Fix: Reuse existing regex object with reset lastIndex for global patterns
Impact: Eliminates unnecessary RegExp construction
// For backward compatibility, also get single text for matchDetails | |
const textToMatch = getText(context, eventType); | |
const singleMatch = new RegExp(regexPattern).exec(textToMatch); | |
// For backward compatibility, also get single text for matchDetails | |
const textToMatch = getText(context, eventType); | |
regex.lastIndex = 0; // Reset for global regex | |
const singleMatch = regex.exec(textToMatch); |
plugins/default/regexMatch.ts
Outdated
textExcerpt: | ||
textToMatch.length > 100 | ||
textToMatch?.length > 100 | ||
? textToMatch.slice(0, 100) + '...' | ||
: textToMatch, |
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.
🐛 Bug Fix
Issue: Potential null pointer exception when textToMatch
is null/undefined
Fix: Add null check before accessing length property
Impact: Prevents runtime crash when text content is missing
textExcerpt: | |
textToMatch.length > 100 | |
textToMatch?.length > 100 | |
? textToMatch.slice(0, 100) + '...' | |
: textToMatch, | |
textExcerpt: | |
textToMatch && textToMatch.length > 100 | |
? textToMatch.slice(0, 100) + '...' | |
: textToMatch, |
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Description
This PR enhances the Regex Match plugin by adding an optional redaction feature.
Developers can now configure the plugin to not only detect regex matches but also redact matching text with a replacement string (default:
[REDACTED]
).Motivation
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
No UI impact – plugin configuration extended with
redact
andredactText
parameters.Checklist
Related Issues
Fixes and closes #1291