Add workflow option to stats command - #247
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe ChangesPer-workflow statistics reporting
Estimated code review effort: 3 (Moderate) | ~22 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3fb5199 to
b134da4
Compare
66ed189 to
51fc334
Compare
![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
🧹 Nitpick comments (2)
lib/cli/stats.js (2)
370-371: 💤 Low valueMisleading parameter name
templatesInWorkflow.The parameter is named
templatesInWorkflowbut it actually receives a full workflow object (which is then passed toyamlFilesActivity). The naming is inconsistent with the actual usage. Consider renaming toworkflowfor clarity.-async function getYamlSummary(sinceDate, templatesInWorkflow) { - const yamlActivity = await yamlFilesActivity(sinceDate, templatesInWorkflow); +async function getYamlSummary(sinceDate, workflow) { + const yamlActivity = await yamlFilesActivity(sinceDate, workflow);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cli/stats.js` around lines 370 - 371, The parameter name templatesInWorkflow in function getYamlSummary is misleading because it actually expects a full workflow object passed into yamlFilesActivity; rename the parameter to workflow (update the function signature of getYamlSummary and all internal references) and update any call sites to pass the same workflow variable name so usage is consistent with the actual object being passed to yamlFilesActivity.
319-344: 💤 Low valueNo defensive checks for missing workflow properties.
If a workflow JSON file is malformed or missing the
templatesproperty (or its sub-properties likereconciliations,exports,accounts), the code will throw with an unclear error. Consider adding defensive checks or clearer error messages.💡 Optional: Add defensive validation
// Fetch workflow // Assume no empty items if present within the workflow. + if (!workflow.templates) { + consola.error(`Workflow "${workflow.name || 'unknown'}" is missing 'templates' property`); + process.exit(1); + } summary.workflow_name = workflow.name; // Reconciliations - const reconciliationsInWorkflow = workflow.templates.reconciliations; + const reconciliationsInWorkflow = workflow.templates.reconciliations || []; // ... - const exportFilesInWorkflow = workflow.templates.exports; + const exportFilesInWorkflow = workflow.templates.exports || []; // ... - const accountTemplatesInWorkflow = workflow.templates.accounts; + const accountTemplatesInWorkflow = workflow.templates.accounts || [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cli/stats.js` around lines 319 - 344, The code assumes workflow.templates and its sub-properties exist (e.g., workflow.templates.reconciliations, .exports, .accounts), which can throw on malformed workflow JSON; update the start of this block to defensively validate workflow and its template arrays (or normalize them) before using them: check that workflow is truthy, that workflow.templates is an object, and that workflow.templates.reconciliations, workflow.templates.exports, workflow.templates.accounts are arrays (fallback to [] if missing) or throw a clear, descriptive error; ensure subsequent uses of reconciliationsInWorkflow, exportFilesInWorkflow, accountTemplatesInWorkflow, and calls to listExternallyManagedTemplates/countYamlFiles use these validated/normalized variables so percentageRoundTwo and .length won't fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cli/stats.js`:
- Around line 23-26: The code currently sets workflowsFolder and populates
workflowHandles using fs.readdirSync which will throw if the "workflows"
directory is missing; update the logic around the
workflowsFolder/workflowHandles assignment (the workflowsFolder const and
workflowHandles variable) to handle a missing folder by either checking
fs.existsSync(workflowsFolder) before calling fs.readdirSync or wrapping
readdirSync in a try/catch and falling back to an empty array, and ensure any
caught error is handled or logged rather than allowed to crash the process.
- Around line 55-58: The TEMPLATE_PATTERN is built from raw template names
(templatesInWorkflow) so regex metacharacters in names break the pattern; before
joining, escape each template string (e.g., implement or use an escapeRegExp
helper and map over templatesInWorkflow to escape chars like . * + ? ^ $ ( ) [ ]
{ } | \ /) then join the escaped names to build TEMPLATE_PATTERN and proceed to
construct YAML_EXPRESSION and fileTypeRegExp from that escaped pattern (refer to
templatesInWorkflow, TEMPLATE_PATTERN, YAML_EXPRESSION, fileTypeRegExp).
In `@lib/utils/fsUtils.js`:
- Around line 483-495: getWorkflow currently uses the raw CLI-provided
workflowHandle to build file paths allowing path traversal; update getWorkflow
to validate and sanitize workflowHandle (e.g., reject any path separators, "..",
or characters outside a safe whitelist like /^[A-Za-z0-9_-]+$/) and return a
clear error instead of accepting unsafe names, then apply the same sanitization
or safe-filename conversion in saveWorkflowOverviewToFile before constructing
CSV/output paths so both functions (getWorkflow and saveWorkflowOverviewToFile)
never join untrusted input into filesystem paths.
---
Nitpick comments:
In `@lib/cli/stats.js`:
- Around line 370-371: The parameter name templatesInWorkflow in function
getYamlSummary is misleading because it actually expects a full workflow object
passed into yamlFilesActivity; rename the parameter to workflow (update the
function signature of getYamlSummary and all internal references) and update any
call sites to pass the same workflow variable name so usage is consistent with
the actual object being passed to yamlFilesActivity.
- Around line 319-344: The code assumes workflow.templates and its
sub-properties exist (e.g., workflow.templates.reconciliations, .exports,
.accounts), which can throw on malformed workflow JSON; update the start of this
block to defensively validate workflow and its template arrays (or normalize
them) before using them: check that workflow is truthy, that workflow.templates
is an object, and that workflow.templates.reconciliations,
workflow.templates.exports, workflow.templates.accounts are arrays (fallback to
[] if missing) or throw a clear, descriptive error; ensure subsequent uses of
reconciliationsInWorkflow, exportFilesInWorkflow, accountTemplatesInWorkflow,
and calls to listExternallyManagedTemplates/countYamlFiles use these
validated/normalized variables so percentageRoundTwo and .length won't fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25e76814-59ef-4417-9c64-b5134a0b6c9a
📒 Files selected for processing (3)
bin/cli.jslib/cli/stats.jslib/utils/fsUtils.js
| fs.mkdirSync("stats"); | ||
| } | ||
| if (!fs.existsSync(CSV_PATH)) { | ||
| fs.writeFileSync(CSV_PATH, ROW_HEADER, (err) => { |
There was a problem hiding this comment.
fs.writeFileSync silently ignores the callback passed as the third argument.
writeFileSync is synchronous and its third parameter is an options object (encoding, mode, flag) — not an error handler. The callback (err) => { consola.error(err); } is coerced to an options object and silently discarded. Any write failure (e.g. permission denied, disk full) throws an uncaught synchronous exception rather than being handled. The same issue exists at line 592 in saveOverviewToFile.
Remove the callback and either let the exception propagate naturally, or wrap the call in a try/catch.
| const FOLDER = fsUtils.FOLDERS[templateType]; | ||
| const YAML_EXPRESSION = `.*${FOLDER}/.*/tests/.*_liquid_test.*.y(a)?ml`; | ||
| const TEMPLATE_PATTERN = templatesInWorkflow ? `(${templatesInWorkflow.join("|")})` : `.*`; | ||
| // Issue with counting multiple yml files in the same tests folder? |
There was a problem hiding this comment.
Leftover open question in a comment (Issue with counting multiple yml files in the same tests folder?). Either confirm this is not a problem and remove the comment, or document/fix the edge case.
| }; | ||
|
|
||
| // Fetch workflow | ||
| // Assume no empty items if present within the workflow. |
There was a problem hiding this comment.
Global vs workflow stats count different populations.
generateOverview counts only non-empty templates (listNonEmptyTemplates), while workflow mode counts every handle listed in the workflow JSON. That is probably fine, but worth documenting in the CLI help or PR description so users do not compare the two totals directly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/cli/utils.js (1)
61-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for date-validation boundaries.
Cover a valid leap day, malformed input, impossible dates such as
2024-02-31, and theprocess.exit(1)failure path.tests/lib/cli/utils.test.jsalready mocksconsola, making it suitable for these cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cli/utils.js` around lines 61 - 76, Add focused tests in utils.test.js for checkDateFormat covering a valid leap day, malformed input, an impossible date such as 2024-02-31, and the process.exit(1) failure path. Reuse the existing consola mock and assert both validation outcomes and the expected exit behavior without changing checkDateFormat.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/cli/utils.js`:
- Around line 61-76: Add focused tests in utils.test.js for checkDateFormat
covering a valid leap day, malformed input, an impossible date such as
2024-02-31, and the process.exit(1) failure path. Reuse the existing consola
mock and assert both validation outcomes and the expected exit behavior without
changing checkDateFormat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41da8d3a-f73d-4466-a831-193bb82f0a10
📒 Files selected for processing (2)
bin/cli.jslib/cli/utils.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/cli.js
1f856a2 to
6bf07a4
Compare
6bf07a4 to
1817f89
Compare
2 participants
Fixes # (link to the corresponding issue if applicable)
Description
Include a summary of the changes made
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist
Notes
Plan
getWorkflowTemplateSummary(workflowHandle)functionlistWorkflowTemplatesfunction which can take different classes?Testing notes
ymlfiles in the sametestsfolder so may need to update the Regex expression:.*${FOLDER}/${templatePattern}/tests/.*_liquid_test.*.y(a)?ml. Resolved by temporarily deleting "extra" tests in test branchQuick Notes
clifolder?getTemplatesSummary--> Can it be co-opted for the purposes of this workflow? Take the "Reconciliations" section as an example!