Conversation
WalkthroughAdds a shared data-x configuration, registers ten new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
@AlbertSmit Cool! Laat nog iemand anders het Google Docsje checken met de |
… to enforce `data-x` attributes on sectioning elements and naming conventions for forms.
|
(dit ligt nu bij Paul, ik heb het |
…ve several legacy `data-x` rules, and update `data-x-onpage-action-format`.
|
@coderabbitai give this another go. |
|
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In `@rules/data-x-cta-prefix/readme.md`:
- Line 7: Update the README for the data-x-cta-prefix rule to use the standard
verb form "sign up" instead of "signup": locate the sentence listing common CTA
actions ("apply, submit, register, signup, download, contact, etc.") in
rules/data-x-cta-prefix/readme.md and replace "signup" with "sign up" so the CTA
verbs are consistently presented; ensure spacing and punctuation remain
consistent with the surrounding list.
In `@rules/data-x-latin-only/index.js`:
- Around line 6-29: The error message in messages.nonLatinDataX states only a–z
are allowed but the regex latinPattern (/^[a-zA-Z0-9\-_]+$/) permits A–Z as
well; update the message to reflect the actual allowed character set (A–Z, a–z,
0–9, hyphens, underscores) or change latinPattern to enforce lowercase only.
Locate messages.nonLatinDataX and latinPattern in the rule (referenced by
nonLatinDataX, latinPattern, JSXOpeningElement, getLiteralPropValue) and make
the message and regex consistent so the reported text matches the implemented
validation.
In `@rules/data-x-sectioning-elements/index.js`:
- Around line 13-14: The JSXOpeningElement handler currently reads
node.name.name which will throw for JSXMemberExpression or JSXNamespacedName;
update the JSXOpeningElement visitor to guard by checking that node.name exists
and node.name.type === 'JSXIdentifier' before accessing node.name.name (or
otherwise bail/return early), so the code that sets elementName only runs for
identifier names; reference the JSXOpeningElement function and the elementName
variable when applying this guard.
In `@rules/data-x-toggle-prefix/index.js`:
- Around line 29-48: The toggle detection currently tests dataXValue against
togglePatterns before stripping open/close/show/hide prefixes, so values like
"open-accordion" are missed; modify the logic in
rules/data-x-toggle-prefix/index.js to first normalize dataXValue by removing
/^(open-|close-|show-|hide-)/, then run togglePatterns.test on the normalized
value, and preserve existing behavior around
ariaExpanded/getProp/hasAriaExpanded and reporting via context.report with
dataXProp and the computed baseValue.
🧹 Nitpick comments (6)
lib/data-x-config.js (1)
37-53: Avoidthis-based helpers to prevent destructuring bugs.
If consumers destructure these helpers,thisbecomes undefined and the methods break. Prefer referencing module exports directly.♻️ Suggested change
getAllActionVerbs() { - return Object.values(this.actionVerbs).flat() + return Object.values(module.exports.actionVerbs).flat() }, getActionVerbPattern() { - return this.getAllActionVerbs().join('|') + return module.exports.getAllActionVerbs().join('|') }, getStandaloneActionVerbPattern() { - return this.standaloneActionVerbs.join('|') + return module.exports.standaloneActionVerbs.join('|') },rules/data-x-cta-prefix/index.js (1)
1-33: Consider deriving allowed prefixes from shared config to avoid drift.
Hard-coded prefix checks can diverge fromlib/data-x-config.jsover time.♻️ Suggested change
-const getProp = require('jsx-ast-utils/getProp') -const getLiteralPropValue = require('jsx-ast-utils/getLiteralPropValue') +const getProp = require('jsx-ast-utils/getProp') +const getLiteralPropValue = require('jsx-ast-utils/getLiteralPropValue') +const { prefixes } = require('../../lib/data-x-config') + +const allowedPrefixes = [ + prefixes.cta, + prefixes.clickout, + prefixes.toggle, + prefixes.link, + 'nav-', + 'scroll-', +] @@ - if ( - dataXValue.startsWith('cta-') || - dataXValue.startsWith('nav-') || - dataXValue.startsWith('clickout-') || - dataXValue.startsWith('toggle-') || - dataXValue.startsWith('scroll-') - ) { + if (allowedPrefixes.some((prefix) => dataXValue.startsWith(prefix))) { return }rules/data-x-sectioning-elements/index.js (1)
16-17: ImportsectioningElementsfrom shared config.The
sectioningElementsarray is exported fromlib/data-x-config.jsand used by bothdata-x-requiredanddata-x-sectioning-elementsrules. Importing from the shared configuration instead of hardcoding the array improves consistency and maintainability.♻️ Suggested refactor
const hasProp = require('jsx-ast-utils/hasProp') +const { sectioningElements } = require('../../lib/data-x-config') module.exports = { // ... create(context) { return { JSXOpeningElement(node) { const elementName = node.name.type === 'JSXIdentifier' ? node.name.name : null if (!elementName) return - // Check for sectioning elements defined in documentation - const sectioningElements = ['form', 'section', 'header', 'footer', 'nav', 'main', 'aside'] - if (!sectioningElements.includes(elementName)) returnrules/position-center/index.js (1)
33-39: TemplateLiteral with expressions may bypass the check.The current logic only examines the first quasi, so a template literal with expressions (e.g.,
`${'center'}`or`cen${'ter'}`) would not be flagged. This is likely an uncommon edge case, but worth noting.rules/data-x-onpage-action-format/index.js (1)
32-38: Consider caching the RegExp instances outside the visitor.The
onpageActionPatternandvalidFormatRegExp objects are recreated on everyJSXOpeningElementvisit. For better performance, consider moving them to module scope or memoizing.♻️ Proposed optimization
const verbPattern = dataXConfig.getActionVerbPattern() const standaloneVerbPattern = dataXConfig.getStandaloneActionVerbPattern() + +const onpageActionPattern = new RegExp(`^(cta-)?(${verbPattern})($|-)`, 'i') +const validFormat = new RegExp(`^(cta-)?(${verbPattern})-[a-z0-9_-]+$`, 'i') +const standaloneActionPattern = standaloneVerbPattern + ? new RegExp(`^(cta-)?(${standaloneVerbPattern})$`, 'i') + : null module.exports = { // ... create(context) { return { JSXOpeningElement(node) { // ... - const onpageActionPattern = new RegExp(`^(cta-)?(${verbPattern})($|-)`, 'i') if (!onpageActionPattern.test(dataXValue)) return - const validFormat = new RegExp(`^(cta-)?(${verbPattern})-[a-z0-9_-]+$`, 'i') - - const standaloneActionPattern = standaloneVerbPattern - ? new RegExp(`^(cta-)?(${standaloneVerbPattern})$`, 'i') - : null - - const isStandaloneAction = standaloneActionPattern - ? standaloneActionPattern.test(dataXValue) - : false + const isStandaloneAction = standaloneActionPattern?.test(dataXValue) ?? false if (!validFormat.test(dataXValue) && !isStandaloneAction) {rules/data-x-context/index.js (1)
19-21: Error message references "page template" but rule applies globally.The message says
'Missing required "data-x-context" attribute on {{elementType}} element in page template', but the rule doesn't actually differentiate page templates from other contexts. Either the message should be updated or the rule should be scoped appropriately (e.g., via file path patterns in ESLint config).♻️ Suggested message update
messages: { - missingDataXContext: 'Missing required "data-x-context" attribute on {{elementType}} element in page template', + missingDataXContext: 'Missing required "data-x-context" attribute on {{elementType}} element', invalidDataXContext: '"data-x-context" value "{{value}}" is not allowed. Allowed values: {{allowedValues}}', },
| test('data-x-unique-id', { | ||
| valid: [ | ||
| { code: '<a data-x="nav-home">Home</a>' }, | ||
| { code: '<button data-x="submit">Submit</button>' }, |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rules/data-x-context/index.js`:
- Around line 14-29: The rule currently reports missing data-x-context even when
data-x-context may be supplied via a JSX spread; modify the JSXOpeningElement
visitor (which uses getProp and inspects node.attributes for 'data-x' and
'data-x-context') to first check for any attribute of type JSXSpreadAttribute on
node.attributes and skip validation (return early) if found, so you don't call
context.report for elements where data-x-context could be provided through a
spread.
🧹 Nitpick comments (1)
rules/data-x-context/test.js (1)
4-22: Add a test case for JSX spread attributes to define intended behavior.Right now the suite only covers explicit attributes. A spread case will lock down whether the rule should skip or still enforce when
data-x-contextmight be provided via{...props}.
| JSXOpeningElement(node) { | ||
| const elementName = node.name.name | ||
|
|
||
| // Only check <a> and <button> elements | ||
| if (elementName !== 'a' && elementName !== 'button') return | ||
|
|
||
| const dataXProp = getProp(node.attributes, 'data-x') | ||
| if (!dataXProp) return | ||
|
|
||
| const dataXContextProp = getProp(node.attributes, 'data-x-context') | ||
| if (!dataXContextProp) { | ||
| context.report({ | ||
| node, | ||
| messageId: 'missingDataXContext', | ||
| data: { elementType: elementName } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "index.js" -path "*/data-x-context/*" | head -5Repository: kaliberjs/eslint-plugin
Length of output: 98
🏁 Script executed:
find . -type f -name "*.js" | grep -i "data-x-context"Repository: kaliberjs/eslint-plugin
Length of output: 129
🏁 Script executed:
git ls-files | grep -i "data-x"Repository: kaliberjs/eslint-plugin
Length of output: 1143
🏁 Script executed:
git ls-files | head -20Repository: kaliberjs/eslint-plugin
Length of output: 598
🏁 Script executed:
ls -laRepository: kaliberjs/eslint-plugin
Length of output: 1168
🏁 Script executed:
find . -type d -name "rules" | head -5Repository: kaliberjs/eslint-plugin
Length of output: 256
🏁 Script executed:
fd "data-x-context" --type fRepository: kaliberjs/eslint-plugin
Length of output: 49
🏁 Script executed:
find . -type f -path "*/rules/*" -name "*.js" | head -20Repository: kaliberjs/eslint-plugin
Length of output: 853
🏁 Script executed:
ls -la && pwdRepository: kaliberjs/eslint-plugin
Length of output: 1187
🏁 Script executed:
rg "getProp" --type js -l | head -10Repository: kaliberjs/eslint-plugin
Length of output: 400
🏁 Script executed:
rg "jsx-ast-utils" --type js | head -20Repository: kaliberjs/eslint-plugin
Length of output: 1895
🏁 Script executed:
rg "JSXSpreadAttribute" --type js -A 5 -B 5 | head -50Repository: kaliberjs/eslint-plugin
Length of output: 3084
🌐 Web query:
jsx-ast-utils getProp JSXSpreadAttribute behavior documentation
💡 Result:
In jsx-ast-utils, getProp(props, prop, options) primarily returns the JSXAttribute node for a named attribute (case-insensitive by default), or undefined if not found. [1]
Behavior with JSXSpreadAttribute ({...x})
getProp has special handling for spread attributes only when the spread argument is an object literal:
- If it encounters a
JSXSpreadAttributewhoseargumentis anObjectExpression(e.g.<Comp {...{ foo: 1 }} />), it will search that object’spropertiesfor a matching key and, if found, convert that objectPropertyinto a syntheticJSXAttributeand return it. [2] - If the spread argument is not an object literal (e.g.
<Comp {...props} />),getPropcannot inspect it, so it won’t “find” props that might exist inside that runtime object. [2] - It also explicitly does not resolve
keyfrom object-literal spreads (it checkspropToFind !== 'key'), referencing the React RFC aboutkeynot being a normal prop. [2]
Contrast: hasProp and spread
hasProp has a spreadStrict option (default true): if a spread exists, it assumes the searched prop is not in the spread unless you set spreadStrict: false. [3]
Sources
[1] jsx-ast-utils README (API: getProp) https://www.npmjs.com/package/jsx-ast-utils
[2] src/getProp.js implementation (spread/object-literal handling + key exception) https://app.unpkg.com/jsx-ast-utils@3.3.5/files/src/getProp.js
[3] src/hasProp.js implementation (spreadStrict) https://app.unpkg.com/jsx-ast-utils@3.3.5/files/src/hasProp.js
🏁 Script executed:
cat package.json 2>/dev/null | grep -A 5 -B 5 "jsx-ast-utils" || echo "No package.json or jsx-ast-utils not found"Repository: kaliberjs/eslint-plugin
Length of output: 299
🏁 Script executed:
ls -la rules/data-x-context/index.js 2>/dev/null || echo "File not found at specified path"Repository: kaliberjs/eslint-plugin
Length of output: 144
🏁 Script executed:
cat -n rules/data-x-context/index.jsRepository: kaliberjs/eslint-plugin
Length of output: 1173
🏁 Script executed:
cat -n rules/data-x-context/test.js | head -100Repository: kaliberjs/eslint-plugin
Length of output: 998
🏁 Script executed:
cat rules/data-x-context/test.jsRepository: kaliberjs/eslint-plugin
Length of output: 837
🏁 Script executed:
cat -n rules/jsx-key/index.js | grep -A 20 -B 5 "JSXSpreadAttribute"Repository: kaliberjs/eslint-plugin
Length of output: 629
🏁 Script executed:
cat rules/data-x-context/readme.mdRepository: kaliberjs/eslint-plugin
Length of output: 671
Skip validation when JSXSpreadAttribute is present to avoid false positives.
When data-x is explicit but data-x-context is provided via {...props}, getProp won't detect it from the spread, causing false positives. The test suite currently lacks coverage for spread attribute scenarios. Add a guard to skip validation when a spread exists.
Proposed guard
JSXOpeningElement(node) {
const elementName = node.name.name
// Only check <a> and <button> elements
if (elementName !== 'a' && elementName !== 'button') return
+
+ const hasSpread = node.attributes.some(attr => attr.type === 'JSXSpreadAttribute')
+ if (hasSpread) return🤖 Prompt for AI Agents
In `@rules/data-x-context/index.js` around lines 14 - 29, The rule currently
reports missing data-x-context even when data-x-context may be supplied via a
JSX spread; modify the JSXOpeningElement visitor (which uses getProp and
inspects node.attributes for 'data-x' and 'data-x-context') to first check for
any attribute of type JSXSpreadAttribute on node.attributes and skip validation
(return early) if found, so you don't call context.report for elements where
data-x-context could be provided through a spread.
rules/data-x-context/readme.md
Outdated
There was a problem hiding this comment.
Maybe dit niet zo strak enforcen dit gaat er voor zorgen dat het veel contextuele knowledge moet hebben bij het toevoegen van buttons en dat het meer frustratie geeft dan helpt.
4 participants
Note
Dit zit nu in beta
1.1.0-beta.0als test-run.OM als developer doc:
Gebaseerd op dit:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores