Improved code coverage by removing dead code, extracting shared utilities, and added tests#723
Conversation
…ties, and added tests
WalkthroughThis pull request refactors the codebase to centralize rule-based linting logic into a shared utility module. A new file Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/checks/100-custom-template-settings-usage.js (1)
42-48: Missingoptionsproperty inapplyRulecall.The
lib/checks/110-page-builder-usage.jspassesoptionstoapplyRule, but this file does not. While the currentisEnabledfunction for GS100 doesn't useoptions, the inconsistency could cause issues if future rules need access tooptions.checkVersionor other settings.Proposed fix for consistency
_.each(rules, function (check, ruleCode) { applyRule({ code: ruleCode, ...check, - ...ruleImplementations[ruleCode] + ...ruleImplementations[ruleCode], + options }, theme); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/checks/100-custom-template-settings-usage.js` around lines 42 - 48, The loop calling applyRule over rules omits the options property, causing inconsistency with other checks (e.g., 110-page-builder-usage) and potential future breakage; update the applyRule invocation inside _.each so it passes options (the same options object used elsewhere) alongside code, check, and ruleImplementations[ruleCode] — e.g., include options: options — so that applyRule, and any isEnabled implementations like GS100, receive options.checkVersion and other settings.lib/utils/check-utils.js (2)
6-18: Consider removing unnecessarygflag from regex.The
g(global) flag on line 10 is unnecessary when using.match()purely to check if a string matches a pattern. SinceruleCode.match(ruleRegex)is used as a boolean check, thegflag adds no value and could cause subtle issues if the regex were reused (due tolastIndexstate).Suggested simplification
- const ruleRegex = new RegExp('^' + id + '-.*', 'g'); + const ruleRegex = new RegExp('^' + id + '-');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/check-utils.js` around lines 6 - 18, In getRules, the RegExp for ruleRegex uses the unnecessary 'g' flag which can cause stateful behavior; change the pattern creation in getRules (ruleRegex) to omit the 'g' flag (e.g., new RegExp('^' + id + '-.*')) and/or use RegExp.prototype.test(ruleCode) or String.prototype.match without the global flag when checking ruleCode.match(ruleRegex) so the match is a simple boolean and not affected by lastIndex.
75-82: Mutating the caller'srulesobject may cause unintended side effects.Line 81 modifies the
rulesobject passed by the caller. If a caller reuses the samerulesobject across multiple calls, it will accumulate themark-used-partialsentry. While currently benign (the condition checks for existence), this pattern could mask bugs or cause confusion.Consider creating a shallow copy before mutation:
Defensive copy suggestion
function parseWithAST({theme, log, file, rules, callback, partialVerificationCache}) { const linter = new ASTLinter(); + + // Avoid mutating the caller's rules object + const effectiveRules = {...rules}; // This rule is needed to find partials // Partials are needed for a full parsing - if (!rules['mark-used-partials']) { - rules['mark-used-partials'] = require(`../ast-linter/rules/mark-used-partials`); + if (!effectiveRules['mark-used-partials']) { + effectiveRules['mark-used-partials'] = require(`../ast-linter/rules/mark-used-partials`); }Then use
effectiveRulesin thelinter.verify()call on line 100.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/check-utils.js` around lines 75 - 82, In parseWithAST, avoid mutating the caller's rules object: create a shallow copy (e.g., effectiveRules = {...rules}) and, if mark-used-partials is missing, add it to effectiveRules instead of rules; then pass effectiveRules to ASTLinter.verify (the linter.verify call) so callers' rules remain unchanged and you still ensure the mark-used-partials rule is present for parsing.test/check-utils.test.js (1)
153-196: Consider adding a test forgetRules.The module exports
getRules,getLogger,applyRule, andparseWithAST, but tests only cover the latter three. Adding coverage forgetRuleswould ensure the regex-based rule filtering and version resolution work as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/check-utils.test.js` around lines 153 - 196, Add a new test case exercising getRules: call getRules with a mock rules object containing multiple rule entries (some matching and some not matching a provided regex pattern) and with different version metadata (e.g., rule.version or rule.from version ranges), assert that the returned set only includes rules whose keys match the regex filter and that the resolved rule versions follow the expected resolution logic (pick the highest/compatible version or fallback as getRules specifies). Use the existing test harness style (create a fake theme/results if needed and reuse getLogger patterns) and include at least two assertions: one for regex filtering and one for correct version selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ast-linter/rules/lint-no-unknown-custom-theme-select-value-in-match.js`:
- Around line 19-22: The current check for match only handles the 3-argument
form and misses the 2-argument shorthand; update the logic in the match handling
block so it supports both 2- and 3-parameter forms (i.e., treat
node.params.length === 2 or === 3), compute the secondary index accordingly (use
indexSecondParameter = node.params.length === 3 ? 2 : 1), and then use
getCustomSettingName(...) and getCustomSettingValue(...) against node.params[0]
and node.params[indexSecondParameter] so shorthand equality forms like {{`#match`
`@custom.typography` "comic sans"}} are validated again.
In `@lib/checker.js`:
- Around line 9-10: The check() function currently mutates the shared spec by
pushing labs-enabled helper names into spec.knownHelpers and uses _.has to test
flags, causing helpers to remain enabled across calls and treating
{labs:{flag:false}} as enabled; fix by computing the list of extra helpers
inside check() per invocation using labsEnabledHelpers and truthy checks (e.g.,
Boolean(spec.labs && spec.labs[flagName])) instead of mutating spec.knownHelpers
or using _.has, then merge that transient list with the existing known helpers
only for the duration of the check call (do not write back to
spec.knownHelpers); add a regression test that calls check() twice in one
process and a test where spec.labs.testFlag is explicitly false to assert the
helper is not enabled.
---
Nitpick comments:
In `@lib/checks/100-custom-template-settings-usage.js`:
- Around line 42-48: The loop calling applyRule over rules omits the options
property, causing inconsistency with other checks (e.g., 110-page-builder-usage)
and potential future breakage; update the applyRule invocation inside _.each so
it passes options (the same options object used elsewhere) alongside code,
check, and ruleImplementations[ruleCode] — e.g., include options: options — so
that applyRule, and any isEnabled implementations like GS100, receive
options.checkVersion and other settings.
In `@lib/utils/check-utils.js`:
- Around line 6-18: In getRules, the RegExp for ruleRegex uses the unnecessary
'g' flag which can cause stateful behavior; change the pattern creation in
getRules (ruleRegex) to omit the 'g' flag (e.g., new RegExp('^' + id + '-.*'))
and/or use RegExp.prototype.test(ruleCode) or String.prototype.match without the
global flag when checking ruleCode.match(ruleRegex) so the match is a simple
boolean and not affected by lastIndex.
- Around line 75-82: In parseWithAST, avoid mutating the caller's rules object:
create a shallow copy (e.g., effectiveRules = {...rules}) and, if
mark-used-partials is missing, add it to effectiveRules instead of rules; then
pass effectiveRules to ASTLinter.verify (the linter.verify call) so callers'
rules remain unchanged and you still ensure the mark-used-partials rule is
present for parsing.
In `@test/check-utils.test.js`:
- Around line 153-196: Add a new test case exercising getRules: call getRules
with a mock rules object containing multiple rule entries (some matching and
some not matching a provided regex pattern) and with different version metadata
(e.g., rule.version or rule.from version ranges), assert that the returned set
only includes rules whose keys match the regex filter and that the resolved rule
versions follow the expected resolution logic (pick the highest/compatible
version or fallback as getRules specifies). Use the existing test harness style
(create a fake theme/results if needed and reuse getLogger patterns) and include
at least two assertions: one for regex filtering and one for correct version
selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c270686-f137-4681-b5a6-b72758dcdb09
📒 Files selected for processing (13)
lib/ast-linter/linter.jslib/ast-linter/rules/lint-no-unknown-custom-theme-select-value-in-match.jslib/checker.jslib/checks/100-custom-template-settings-usage.jslib/checks/110-page-builder-usage.jslib/checks/120-no-unknown-globals.jslib/utils/check-utils.jslib/utils/labs-enabled-helpers.jstest/ast-linter.test.jstest/check-utils.test.jstest/checker.test.jstest/fixtures/ast-linter/nested-async-helpers.hbstest/fixtures/ast-linter/prev-next-outside-post.hbs
💤 Files with no reviewable changes (1)
- lib/checks/120-no-unknown-globals.js
1 participant
no ref
These updates are to prep for moving from mocha to vitest.
Note
Low Risk
Low risk refactor that mainly deduplicates check execution code and moves labs-helper configuration, with behavior guarded by new unit tests. Minor linter/rule guard removals could surface latent assumptions but should be covered by existing parsing defaults.
Overview
Refactors theme checks by extracting duplicated
getRules/getLogger/applyRule/parseWithASTlogic from checks100and110into a sharedlib/utils/check-utils.js, and moves the labs-flagged helper mapping out ofchecker.jsintolib/utils/labs-enabled-helpers.js.Simplifies AST-linter internals by removing dead guards around scanner context properties and tightening the
match-select-value rule to only handle the 3-param form; also removes an unreachabletheme.helpersinitialization in120-no-unknown-globals.Adds targeted tests for the new utilities, labs-enabled helper injection, and additional AST-linter rule coverage (with new
.hbsfixtures for nested async helpers andprev_postcontext validation).Written by Cursor Bugbot for commit 322aa29. This will update automatically on new commits. Configure here.