Merged
Conversation
WalkthroughThis pull request introduces a new "split-members" feature that enables downloading Ghost members based on a filter criteria and splitting them into two balanced groups for export. The feature is implemented across multiple layers: CLI command registration in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tasks/split-members.js (1)
50-53: Defensively initializectx.errorsin the task context.On Line 76,
ctx.errors.push(error)assumes callers always pass{errors: []}. Initializing once ininitialisemakes the task runner safer for direct reuse.Proposed fix
ctx.members = []; ctx.membersA = []; ctx.membersB = []; + ctx.errors = Array.isArray(ctx.errors) ? ctx.errors : [];Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/split-members.js` around lines 50 - 53, The task assumes ctx.errors exists but it may not; update the initialise logic (where ctx.members, ctx.membersA and ctx.membersB are set) to defensively initialize ctx.errors = [] so subsequent code like the push in the task (ctx.errors.push(error)) cannot blow up; locate the initialise function in tasks/split-members.js and add the errors array initialization alongside the existing member initializations.
🤖 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/split-members.js`:
- Around line 66-78: The catch block currently logs errors with
ui.log.error('Done with errors', context.errors) but execution continues and
still prints success messages; either return immediately from the catch (so the
function exits) or guard the success logging by checking that there are no
errors (e.g., if (!context.errors || context.errors.length === 0) {
ui.log.ok(...) ... }). Update the code around the catch handling and the success
logging (references: the catch block that calls ui.log.error and the subsequent
ui.log.ok/ui.log.info calls) so that success output is only emitted when the
task completed without errors.
In `@prompts/split-members.js`:
- Around line 18-37: The module exports currently include choice and run but not
options; extract the inquirer question array (the array passed to
inquirer.prompt — containing the objects for filterURL, output, baseName) into a
top-level constant named options and export it alongside choice and run so the
prompt contract is satisfied; update any references so run uses the options
constant (e.g., call inquirer.prompt(options)) and add options to the
module.exports/exports list.
- Around line 62-74: The catch block currently logs errors via
ui.log.error('Done with errors', context.errors) but execution continues and
ui.log.ok(`Split ${total} members into A (${aCount}) and B (${bCount})`) still
runs, producing a false success message; modify the error path in the try/catch
around the task-run (the catch that references context.errors and ui.log.error)
to return early (or set/inspect a failure flag) so the success logging
(ui.log.ok) is gated and only runs when no errors occurred, ensuring that when
the catch executes the function exits before computing total/aCount/bCount and
emitting the success log.
In `@tasks/split-members.js`:
- Around line 46-48: The parsed filter from
parseFilterFromURL(options.filterURL) may be null, so before assigning to
ctx.args.filter or continuing, check the return value and handle invalid/empty
parses: call parseFilterFromURL(options.filterURL), if it returns null log or
throw a descriptive error (or exit the task) and do not proceed with splitting;
only assign to ctx.args.filter and continue when the parse result is a non-null
valid filter. Ensure this validation is applied around the current block that
references options.filterURL and ctx.args.filter so the task won't fetch/split
all members when the URL parsing fails.
---
Nitpick comments:
In `@tasks/split-members.js`:
- Around line 50-53: The task assumes ctx.errors exists but it may not; update
the initialise logic (where ctx.members, ctx.membersA and ctx.membersB are set)
to defensively initialize ctx.errors = [] so subsequent code like the push in
the task (ctx.errors.push(error)) cannot blow up; locate the initialise function
in tasks/split-members.js and add the errors array initialization alongside the
existing member initializations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73d26d97-490c-45ce-a46f-13d793ef10a3
📒 Files selected for processing (7)
README.mdbin/cli.jscommands/interactive.jscommands/split-members.jsprompts/index.jsprompts/split-members.jstasks/split-members.js
Comment on lines
+66
to
+78
| } catch (error) { | ||
| ui.log.error('Done with errors', context.errors); | ||
| } | ||
|
|
||
| const total = context.members ? context.members.length : 0; | ||
| const aCount = context.membersA ? context.membersA.length : 0; | ||
| const bCount = context.membersB ? context.membersB.length : 0; | ||
|
|
||
| ui.log.ok(`Split ${total} members into A (${aCount}) and B (${bCount}) in ${Date.now() - timer}ms.`); | ||
| ui.log.info(`Files written to: ${argv.output || '.'}`); | ||
| ui.log.info(` ${argv.baseName || 'members'}-all.csv (${total} members)`); | ||
| ui.log.info(` ${argv.baseName || 'members'}-a.csv (${aCount} members)`); | ||
| ui.log.info(` ${argv.baseName || 'members'}-b.csv (${bCount} members)`); |
There was a problem hiding this comment.
Avoid logging success output when the task runner fails.
After Line 67 logs errors, execution continues to Line 74 and reports success. Return from the catch path (or guard success logs) to prevent false-positive completion messages.
Proposed fix
try {
let runner = splitMembers.getTaskRunner(argv);
await runner.run(context);
} catch (error) {
- ui.log.error('Done with errors', context.errors);
+ ui.log.error('Done with errors', context.errors.length ? context.errors : [error]);
+ return;
}📝 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
| } catch (error) { | |
| ui.log.error('Done with errors', context.errors); | |
| } | |
| const total = context.members ? context.members.length : 0; | |
| const aCount = context.membersA ? context.membersA.length : 0; | |
| const bCount = context.membersB ? context.membersB.length : 0; | |
| ui.log.ok(`Split ${total} members into A (${aCount}) and B (${bCount}) in ${Date.now() - timer}ms.`); | |
| ui.log.info(`Files written to: ${argv.output || '.'}`); | |
| ui.log.info(` ${argv.baseName || 'members'}-all.csv (${total} members)`); | |
| ui.log.info(` ${argv.baseName || 'members'}-a.csv (${aCount} members)`); | |
| ui.log.info(` ${argv.baseName || 'members'}-b.csv (${bCount} members)`); | |
| } catch (error) { | |
| ui.log.error('Done with errors', context.errors.length ? context.errors : [error]); | |
| return; | |
| } | |
| const total = context.members ? context.members.length : 0; | |
| const aCount = context.membersA ? context.membersA.length : 0; | |
| const bCount = context.membersB ? context.membersB.length : 0; | |
| ui.log.ok(`Split ${total} members into A (${aCount}) and B (${bCount}) in ${Date.now() - timer}ms.`); | |
| ui.log.info(`Files written to: ${argv.output || '.'}`); | |
| ui.log.info(` ${argv.baseName || 'members'}-all.csv (${total} members)`); | |
| ui.log.info(` ${argv.baseName || 'members'}-a.csv (${aCount} members)`); | |
| ui.log.info(` ${argv.baseName || 'members'}-b.csv (${bCount} members)`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@commands/split-members.js` around lines 66 - 78, The catch block currently
logs errors with ui.log.error('Done with errors', context.errors) but execution
continues and still prints success messages; either return immediately from the
catch (so the function exits) or guard the success logging by checking that
there are no errors (e.g., if (!context.errors || context.errors.length === 0) {
ui.log.ok(...) ... }). Update the code around the catch handling and the success
logging (references: the catch block that calls ui.log.error and the subsequent
ui.log.ok/ui.log.info calls) so that success output is only emitted when the
task completed without errors.
Comment on lines
+18
to
+37
| const answers = await inquirer.prompt([ | ||
| { | ||
| type: 'input', | ||
| name: 'filterURL', | ||
| message: 'Ghost admin URL with filter (paste from browser):', | ||
| validate: input => input.trim().length > 0 || 'Please provide a filter URL' | ||
| }, | ||
| { | ||
| type: 'input', | ||
| name: 'output', | ||
| message: 'Output directory:', | ||
| default: '.' | ||
| }, | ||
| { | ||
| type: 'input', | ||
| name: 'baseName', | ||
| message: 'Base filename prefix:', | ||
| default: 'members' | ||
| } | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Export prompt options to match the prompt module contract.
This module currently exports choice and run, but not options. Please extract the question array and export it alongside choice and run.
Proposed fix
+const options = [
+ {
+ type: 'input',
+ name: 'filterURL',
+ message: 'Ghost admin URL with filter (paste from browser):',
+ validate: input => input.trim().length > 0 || 'Please provide a filter URL'
+ },
+ {
+ type: 'input',
+ name: 'output',
+ message: 'Output directory:',
+ default: '.'
+ },
+ {
+ type: 'input',
+ name: 'baseName',
+ message: 'Base filename prefix:',
+ default: 'members'
+ }
+];
+
async function run() {
@@
- const answers = await inquirer.prompt([
- {
- type: 'input',
- name: 'filterURL',
- message: 'Ghost admin URL with filter (paste from browser):',
- validate: input => input.trim().length > 0 || 'Please provide a filter URL'
- },
- {
- type: 'input',
- name: 'output',
- message: 'Output directory:',
- default: '.'
- },
- {
- type: 'input',
- name: 'baseName',
- message: 'Base filename prefix:',
- default: 'members'
- }
- ]);
+ const answers = await inquirer.prompt(options);
@@
export default {
choice,
+ options,
run
};As per coding guidelines, "prompts/**/*.js: Define interactive mode prompts in prompts/.js using inquirer with exports for choice, options, and run function".
Also applies to: 77-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prompts/split-members.js` around lines 18 - 37, The module exports currently
include choice and run but not options; extract the inquirer question array (the
array passed to inquirer.prompt — containing the objects for filterURL, output,
baseName) into a top-level constant named options and export it alongside choice
and run so the prompt contract is satisfied; update any references so run uses
the options constant (e.g., call inquirer.prompt(options)) and add options to
the module.exports/exports list.
Comment on lines
+62
to
+74
| } catch (error) { | ||
| ui.log.error('Done with errors', context.errors); | ||
| } | ||
|
|
||
| const total = context.members ? context.members.length : 0; | ||
| const aCount = context.membersA ? context.membersA.length : 0; | ||
| const bCount = context.membersB ? context.membersB.length : 0; | ||
|
|
||
| ui.log.ok(`Split ${total} members into A (${aCount}) and B (${bCount})`); | ||
|
|
||
| if (context.errors && context.errors.length > 0) { | ||
| ui.log.error('Errors:', context.errors); | ||
| } |
There was a problem hiding this comment.
Stop execution after task-run failure to avoid false success output.
On Line 70, success is logged even when Line 63 enters the error path. Return early from catch (or gate success logging) so failures are not reported as successful splits.
Proposed fix
try {
let runner = splitMembers.getTaskRunner(options);
await runner.run(context);
} catch (error) {
- ui.log.error('Done with errors', context.errors);
+ ui.log.error('Done with errors', context.errors.length ? context.errors : [error]);
+ return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prompts/split-members.js` around lines 62 - 74, The catch block currently
logs errors via ui.log.error('Done with errors', context.errors) but execution
continues and ui.log.ok(`Split ${total} members into A (${aCount}) and B
(${bCount})`) still runs, producing a false success message; modify the error
path in the try/catch around the task-run (the catch that references
context.errors and ui.log.error) to return early (or set/inspect a failure flag)
so the success logging (ui.log.ok) is gated and only runs when no errors
occurred, ensuring that when the catch executes the function exits before
computing total/aCount/bCount and emitting the success log.
Comment on lines
+46
to
+48
| if (options.filterURL) { | ||
| ctx.args.filter = parseFilterFromURL(options.filterURL); | ||
| } |
There was a problem hiding this comment.
Validate parsed filter URLs before proceeding.
On Line 47, parseFilterFromURL() can return null; execution then continues and may fetch/split all members instead of the intended filtered subset.
Proposed fix
// Parse filter from URL if provided
if (options.filterURL) {
- ctx.args.filter = parseFilterFromURL(options.filterURL);
+ const parsedFilter = parseFilterFromURL(options.filterURL);
+ if (!parsedFilter) {
+ throw new Error('Invalid --filterURL: missing `filter` query parameter');
+ }
+ ctx.args.filter = parsedFilter;
}📝 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
| if (options.filterURL) { | |
| ctx.args.filter = parseFilterFromURL(options.filterURL); | |
| } | |
| if (options.filterURL) { | |
| const parsedFilter = parseFilterFromURL(options.filterURL); | |
| if (!parsedFilter) { | |
| throw new Error('Invalid --filterURL: missing `filter` query parameter'); | |
| } | |
| ctx.args.filter = parsedFilter; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/split-members.js` around lines 46 - 48, The parsed filter from
parseFilterFromURL(options.filterURL) may be null, so before assigning to
ctx.args.filter or continuing, check the return value and handle invalid/empty
parses: call parseFilterFromURL(options.filterURL), if it returns null log or
throw a descriptive error (or exit the task) and do not proceed with splitting;
only assign to ctx.args.filter and continue when the parse result is a non-null
valid filter. Ensure this validation is applied around the current block that
references options.filterURL and ctx.args.filter so the task won't fetch/split
all members when the URL parsing fails.
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
1 participant
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.
No description provided.