✨ Make find-replace safer, support dry-run mode and more useful --verbose#694
Conversation
WalkthroughAdded validation in commands/find-replace.js to error and abort if Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/find-replace.js`:
- Around line 55-60: The validation branch that checks argv.replace currently
logs an error via ui.log.error and then returns, which can leave the process
exiting with success; in the argv.replace !== null && typeof argv.replace !==
'string' block (where argv.replace and ui.log.error are used) change the control
flow so the command fails with a non-zero exit — for example after logging call
process.exit(1) (or alternatively throw a descriptive Error) so automation sees
a failing exit code instead of a silent return.
- Around line 76-79: The success message in commands/find-replace.js (ui.log.ok)
is emitted unconditionally for replace mode even if runner.run(context) threw;
move the success log so it only executes when runner.run(context) completes
without throwing (e.g., place the ui.log.ok(`Successfully updated
${context.updated.length} strings in ${Date.now() - timer}ms.`) inside the try
block immediately after await runner.run(context), or set/inspect a success flag
returned from runner.run before logging), ensuring it uses the existing
context.updated and timer values and that the catch block continues to handle
and log failures without emitting success.
In `@tasks/find-replace.js`:
- Around line 103-105: The loop over ctx.toUpdate destructures
Object.entries(post.matchesByField) into [field, count] but never uses field,
causing a lint error; change the iteration to avoid the unused identifier by
iterating over values only (e.g., use Object.values(post.matchesByField) or
destructure as [, count]) inside the loop that updates totalMatches so only
count is referenced, keeping the aggregation logic in the same block that
processes ctx.toUpdate and post.matchesByField.
| // Validate --replace: if the flag is present but has no argument, | ||
| // sywac may coerce it to a non-string value (e.g. boolean true). | ||
| if (argv.replace !== null && typeof argv.replace !== 'string') { | ||
| ui.log.error(`--replace requires an argument. Provide '' to replace text with an empty string.`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Invalid --replace usage should fail the command, not silently return.
Right now Line 59 returns after logging, which can still produce a successful process exit code in automation.
Suggested fix
if (argv.replace !== null && typeof argv.replace !== 'string') {
- ui.log.error(`--replace requires an argument. Provide '' to replace text with an empty string.`);
- return;
+ const message = `--replace requires an argument. Provide '' to replace text with an empty string.`;
+ ui.log.error(message);
+ throw new TypeError(message);
}📝 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.
| // Validate --replace: if the flag is present but has no argument, | |
| // sywac may coerce it to a non-string value (e.g. boolean true). | |
| if (argv.replace !== null && typeof argv.replace !== 'string') { | |
| ui.log.error(`--replace requires an argument. Provide '' to replace text with an empty string.`); | |
| return; | |
| } | |
| // Validate --replace: if the flag is present but has no argument, | |
| // sywac may coerce it to a non-string value (e.g. boolean true). | |
| if (argv.replace !== null && typeof argv.replace !== 'string') { | |
| const message = `--replace requires an argument. Provide '' to replace text with an empty string.`; | |
| ui.log.error(message); | |
| throw new TypeError(message); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/find-replace.js` around lines 55 - 60, The validation branch that
checks argv.replace currently logs an error via ui.log.error and then returns,
which can leave the process exiting with success; in the argv.replace !== null
&& typeof argv.replace !== 'string' block (where argv.replace and ui.log.error
are used) change the control flow so the command fails with a non-zero exit —
for example after logging call process.exit(1) (or alternatively throw a
descriptive Error) so automation sees a failing exit code instead of a silent
return.
markstos
force-pushed
the
find-replace-dry-run
branch
from
606062b to
c98d628
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
commands/find-replace.js (2)
55-60:⚠️ Potential issue | 🟠 MajorInvalid
--replaceusage should fail the command, not silently return.Returning after
ui.log.errorallows the process to exit with code 0, which can mislead automation scripts into thinking the command succeeded.Suggested fix
if (argv.replace !== null && typeof argv.replace !== 'string') { - ui.log.error(`--replace requires an argument. Provide '' to replace text with an empty string.`); - return; + const message = `--replace requires an argument. Provide '' to replace text with an empty string.`; + ui.log.error(message); + throw new TypeError(message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/find-replace.js` around lines 55 - 60, The check for invalid argv.replace currently logs an error and returns, which lets the process exit with code 0; change this so the command fails with a non-zero exit: after calling ui.log.error include a terminating failure (e.g. throw an Error or call process.exit(1)) to ensure automation sees the failure; update the branch that tests argv.replace !== null && typeof argv.replace !== 'string' in commands/find-replace.js to stop normal execution and return a non-zero status rather than silently returning.
72-79:⚠️ Potential issue | 🟠 MajorSuccess log can be emitted after errors, and
context.updatedmay be undefined.If
runner.run(context)throws, the catch block logs the error but does not return or re-throw, so execution continues to Line 78. This prints a success message even after failures. Additionally,context.updatedmay not be populated if the error occurs early, causing a runtime exception when accessing.length.Suggested fix
} catch (error) { ui.log.error('Done with errors', context.errors); + return; } if (argv.replace !== null) { // Report success for replace mode - ui.log.ok(`Successfully updated ${context.updated.length} strings in ${Date.now() - timer}ms.`); + ui.log.ok(`Successfully updated ${(context.updated ?? []).length} strings in ${Date.now() - timer}ms.`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/find-replace.js` around lines 72 - 79, The success log can run after errors because runner.run(context) errors are caught but not aborted, and accessing context.updated may throw if undefined; update the catch in the try/catch around runner.run(context) to abort control flow (either re-throw or return) after calling ui.log.error, and when emitting the success message guarded by argv.replace check use a safe length access (e.g., use (context.updated || []).length or verify Array.isArray(context.updated)) to avoid runtime exceptions; refer to runner.run(context), ui.log.error, ui.log.ok, argv.replace, and context.updated when making the change.tasks/find-replace.js (1)
103-107:⚠️ Potential issue | 🟡 MinorFix lint error: unused
fieldvariable in total match aggregation.Line 104 destructures
fieldbut never uses it. UseObject.values()instead to iterate only over counts.Suggested fix
for (const post of ctx.toUpdate) { - for (const [field, count] of Object.entries(post.matchesByField)) { + for (const count of Object.values(post.matchesByField)) { totalMatches += count; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/find-replace.js` around lines 103 - 107, The loop in tasks/find-replace.js that aggregates totalMatches currently destructures "field" and "count" but never uses "field"; update the aggregation to iterate only over the counts (e.g., use Object.values(post.matchesByField) or destructure as [, count]) so the unused "field" variable is removed — adjust the inner loop inside the block iterating ctx.toUpdate where totalMatches is incremented to use values-only iteration and keep the increment logic the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@commands/find-replace.js`:
- Around line 55-60: The check for invalid argv.replace currently logs an error
and returns, which lets the process exit with code 0; change this so the command
fails with a non-zero exit: after calling ui.log.error include a terminating
failure (e.g. throw an Error or call process.exit(1)) to ensure automation sees
the failure; update the branch that tests argv.replace !== null && typeof
argv.replace !== 'string' in commands/find-replace.js to stop normal execution
and return a non-zero status rather than silently returning.
- Around line 72-79: The success log can run after errors because
runner.run(context) errors are caught but not aborted, and accessing
context.updated may throw if undefined; update the catch in the try/catch around
runner.run(context) to abort control flow (either re-throw or return) after
calling ui.log.error, and when emitting the success message guarded by
argv.replace check use a safe length access (e.g., use (context.updated ||
[]).length or verify Array.isArray(context.updated)) to avoid runtime
exceptions; refer to runner.run(context), ui.log.error, ui.log.ok, argv.replace,
and context.updated when making the change.
In `@tasks/find-replace.js`:
- Around line 103-107: The loop in tasks/find-replace.js that aggregates
totalMatches currently destructures "field" and "count" but never uses "field";
update the aggregation to iterate only over the counts (e.g., use
Object.values(post.matchesByField) or destructure as [, count]) so the unused
"field" variable is removed — adjust the inner loop inside the block iterating
ctx.toUpdate where totalMatches is incremented to use values-only iteration and
keep the increment logic the same.
|
Just one linting error, then this is GTG (pending #693, which is in this too) |
markstos
force-pushed
the
find-replace-dry-run
branch
from
c98d628 to
2c01d43
Compare
|
I've pushed a fix to address the linting error. |
Before, if you omitted --replace, the tool would proceed with replacing whatever the --find option was with "null", it seems. Now, it is documented to be safe to omit "--replace". If so, you can get either a summary or --verbose report of what matched. Also, Using --replace with no argument is not considered a mistake of converting the value to null. If you want to replace something with an empty string, you have to explicitly give an empty string.
markstos
force-pushed
the
find-replace-dry-run
branch
from
2c01d43 to
98a7184
Compare
|
Also pushed a fix to remove the "superbytes" commit, which turned out to be a local issue as @PaulAdamDavis correctly guessed. |
2 participants
The module import fix may also show up on this branch because it was necessary to base this branch
on the other just so the project could run at all.
Before, if you omitted --replace, the tool would proceed with replacing whatever the --find option was with "null", it seems.
Now, it is documented to be safe to omit "--replace". If so, you can get either a summary or --verbose report of what matched.
Also, Using --replace with no argument is not considered a mistake of converting the value
to null. If you want to replace something with an empty string, you have to explicitly give an empty string.
Here's an example --verbose dry-run report:
First column is number of matches, second column is where it matched.
Note
Medium Risk
Changes behavior of the
find-replacecontent-editing CLI to avoid unintended replacements and to add a new dry-run reporting path; any logic changes here can affect bulk post updates.Overview
find-replaceis now safer by default. Omitting--replaceno longer performs an implicit replacement; it runs in dry-run mode that reports how many matches were found (and, with-V, a per-post/per-field breakdown).Adds validation to reject
--replacewhen provided without an argument (requiring''for empty-string replacements), gates the "Replacing text" task behind non-dry-run mode, and updates docs/help text to reflect the new workflow.Written by Cursor Bugbot for commit 98a7184. This will update automatically on new commits. Configure here.