chore(internals): add new rules for class inheritance and aria attribute management#92
Merged
Conversation
📝 WalkthroughWalkthroughAdds three new ESLint rules and registers them in configs; integrates ChangesMedia package integration and ESLint infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
coryrylan
changed the title
![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
🤖 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 `@projects/internals/eslint/src/local/no-single-consumer-internal-base.js`:
- Around line 155-165: The barrel-import resolution breaks on Windows due to
hardcoded '/' handling; update getRelativeJsImport and the caller
isExportedFromInternalBarrel to normalize paths to POSIX-style before slicing or
passing to relative(): compute fromDirectory by first normalizing fromFile (use
normalizePath or replace backslashes) then find lastIndexOf('/'), normalize the
toFile (remove backslash extensions) and ensure the value passed as indexFile
(the result of join()) is normalized with normalizePath so relative() and the
specifier logic produce a proper './...' specifier on all platforms.
- Around line 139-153: The current logic in importsClass and countSubclasses
misses aliased imports (e.g., "import { Base as InternalBase }") so subclasses
extending the local alias are not counted; update the file to first parse import
declarations into a local import map that resolves exported symbol names to
their local identifiers (handle named imports with "as" aliases, default
imports, and namespace imports where applicable) and then change countSubclasses
to test for subclasses extending any of the local identifiers from that map
rather than the original className string (use importsClass or a new helper to
build the map and feed the derived local names into the subclass regex
matching).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1a5b9b77-59ea-451f-a8b0-474a3f3fef55
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
knip.config.jsprojects/internals/eslint/README.mdprojects/internals/eslint/src/configs/lit.jsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/eslint/src/local/no-deep-class-inheritance.jsprojects/internals/eslint/src/local/no-deep-class-inheritance.test.jsprojects/internals/eslint/src/local/no-host-managed-aria-attributes.jsprojects/internals/eslint/src/local/no-host-managed-aria-attributes.test.jsprojects/internals/eslint/src/local/no-single-consumer-internal-base.jsprojects/internals/eslint/src/local/no-single-consumer-internal-base.test.jsprojects/site/eleventy.config.jsprojects/site/package.jsonprojects/site/src/_11ty/layouts/docs.11ty.jsprojects/site/src/_11ty/templates/api.jsprojects/site/src/docs/elements/_tabs/examples.11ty.jsprojects/starters/eleventy-ssr/src/index.11ty.js
coryrylan
force-pushed
the
topic-internal-lint
branch
from
672da0b to
af08c63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
projects/internals/eslint/src/local/no-single-consumer-internal-base.js (1)
238-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize joined filesystem paths before computing relative import specifiers.
Line 239 slices on
'/', and Line 273 passes a platform-nativejoin()path into that flow. On Windows, this can generate invalid specifiers and miss consumers.Suggested fix
-import { dirname, join, relative, sep } from 'node:path'; +import { dirname, join, relative, sep } from 'node:path'; ... function getRelativeJsImport(fromFile, toFile) { - const fromDirectory = fromFile.slice(0, fromFile.lastIndexOf('/')); - const withoutExtension = toFile.replace(/\.tsx?$/, '.js'); + const normalizedFromFile = normalizePath(fromFile); + const normalizedToFile = normalizePath(toFile); + const fromDirectory = dirname(normalizedFromFile); + const withoutExtension = normalizedToFile.replace(/\.tsx?$/, '.js'); let specifier = normalizePath(relative(fromDirectory, withoutExtension)); ... function isExportedFromInternalBarrel(rootDir, filename, className) { - const indexFile = join(rootDir, 'src/internal/index.ts'); + const indexFile = normalizePath(join(rootDir, 'src/internal/index.ts')); if (!existsSync(indexFile)) { return false; }Also applies to: 272-279
🤖 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 `@projects/internals/eslint/src/local/no-single-consumer-internal-base.js` around lines 238 - 241, The getRelativeJsImport function builds a relative specifier using paths that may contain platform-specific separators; normalize both inputs to POSIX-style paths before slicing and joining so Windows backslashes don't leak into specifiers. Specifically, ensure the fromFile and toFile (and any usage of join()/relative()) are converted to normalized POSIX paths (e.g., replace backslashes with '/') before deriving fromDirectory, computing withoutExtension, and calling normalizePath/relative so the import specifier is always valid.
🤖 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
`@projects/internals/eslint/src/local/no-single-consumer-internal-base.test.js`:
- Around line 275-279: The helper createFile uses string slicing with '/' to
derive the parent directory which breaks on Windows; change it to use Node's
path.dirname (ensure path is imported) to compute the directory and pass that to
mkdirSync with { recursive: true }, e.g. replace filename.slice(0,
filename.lastIndexOf('/')) with path.dirname(filename) inside the createFile
function to make directory creation cross-platform.
---
Duplicate comments:
In `@projects/internals/eslint/src/local/no-single-consumer-internal-base.js`:
- Around line 238-241: The getRelativeJsImport function builds a relative
specifier using paths that may contain platform-specific separators; normalize
both inputs to POSIX-style paths before slicing and joining so Windows
backslashes don't leak into specifiers. Specifically, ensure the fromFile and
toFile (and any usage of join()/relative()) are converted to normalized POSIX
paths (e.g., replace backslashes with '/') before deriving fromDirectory,
computing withoutExtension, and calling normalizePath/relative so the import
specifier is always valid.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 06dfbb52-e343-4cf3-aaea-496aca9c4c64
📒 Files selected for processing (8)
projects/internals/eslint/src/configs/lit.jsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/eslint/src/local/no-deep-class-inheritance.jsprojects/internals/eslint/src/local/no-deep-class-inheritance.test.jsprojects/internals/eslint/src/local/no-host-managed-aria-attributes.jsprojects/internals/eslint/src/local/no-host-managed-aria-attributes.test.jsprojects/internals/eslint/src/local/no-single-consumer-internal-base.jsprojects/internals/eslint/src/local/no-single-consumer-internal-base.test.js
coryrylan
commented
May 18, 2026
| 'local-typescript/no-deep-class-inheritance': [ | ||
| 'error', | ||
| { maxDepth: 2, allowedRoots: ['HTMLElement', 'LitElement', 'FormControlMixin', 'BaseButton'] } | ||
| ], |
Collaborator
Author
There was a problem hiding this comment.
This is to keep agents from wanting to default to large inheritance chains instead of using composition based solutions
coryrylan
commented
May 18, 2026
| 'local/require-element-definitions': ['error'], | ||
| 'local/require-composed-events': ['error'], | ||
| 'local/no-host-managed-aria-attributes': ['error'], | ||
| 'local/no-single-consumer-internal-base': ['error'], |
Collaborator
Author
There was a problem hiding this comment.
This rule helps prevent agents from creating unnecessary base class abstractions when there is no shared code logic between classes or components.
coryrylan
commented
May 18, 2026
| export default { | ||
| meta: { | ||
| type: 'problem', | ||
| name: 'no-host-managed-aria-attributes', |
Collaborator
Author
There was a problem hiding this comment.
This helps prevent agents from using attribute based aria setters instead of leaning out the element internal APIs which provide better element default behavior.
coryrylan
commented
May 18, 2026
| if (section === 'integrations') return 'integrations'; | ||
| if (section === 'foundations') return 'foundations'; | ||
| if (section === 'patterns') return 'patterns'; | ||
| if (section === 'media') return 'elements'; |
Collaborator
Author
There was a problem hiding this comment.
Fixing some minor missing infra for the media package and build/docs
johnyanarella
approved these changes
May 18, 2026
- Added '@nvidia-elements/media' to the configuration and package.json. - Updated Eleventy configuration to include public component documentation for media. - Enhanced example URL generation to support media components. - Refactored coverage calculation logic in API template for better clarity. - Updated pnpm lockfile to reflect new dependencies and versions. Signed-off-by: Cory Rylan <crylan@nvidia.com>
…ute management - Introduced `no-deep-class-inheritance` rule to limit class inheritance depth, ensuring better maintainability. - Added `no-host-managed-aria-attributes` rule to prevent misuse of ARIA attributes that should be managed through ElementInternals. - Implemented `no-single-consumer-internal-base` rule to disallow internal base classes with fewer than two consumers, promoting better design practices. - Updated ESLint configurations to include the new rules and their respective error handling. Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-internal-lint
branch
from
af08c63 to
45c25e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
projects/internals/eslint/src/local/no-single-consumer-internal-base.test.js (1)
5-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
path.dirnamefor parent-directory creation.Line 277 derives the directory using
'/'slicing, which breaks on Windows path separators and can fail temp file setup.Suggested fix
-import { join } from 'node:path'; +import { dirname, join } from 'node:path'; @@ function createFile(relativePath, content) { const filename = join(rootDir, relativePath); - mkdirSync(filename.slice(0, filename.lastIndexOf('/')), { recursive: true }); + mkdirSync(dirname(filename), { recursive: true }); writeFileSync(filename, content); return filename; }#!/bin/bash # Verify the non-portable pattern exists rg -n "filename\.slice\(0, filename\.lastIndexOf\('/'\)\)" projects/internals/eslint/src/local/no-single-consumer-internal-base.test.js # Demonstrate why '/' slicing fails for Windows-style paths python - <<'PY' from pathlib import PureWindowsPath filename = str(PureWindowsPath(r"C:\repo\src\internal\media-button.ts")) sliced_parent = filename[:filename.rfind('/')] print("filename:", filename) print("sliced_parent:", repr(sliced_parent)) print("expected_parent:", str(PureWindowsPath(filename).parent)) PYAlso applies to: 275-279
🤖 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 `@projects/internals/eslint/src/local/no-single-consumer-internal-base.test.js` at line 5, The test constructs a parent directory by slicing the string filename with '/' which is not cross-platform; replace the manual slice (occurrences of filename.slice(0, filename.lastIndexOf('/')) and any similar uses around the filename variable) with a proper path dirname call (e.g., path.dirname(filename) or import { dirname } from 'path' and use dirname(filename)) so Windows backslashes are handled correctly and temp file setup becomes portable; update any related uses in the same block (lines ~275-279) to use the dirname-based approach and remove the brittle string slicing.projects/internals/eslint/src/local/no-single-consumer-internal-base.js (1)
238-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize path inputs before computing relative JS import specifiers.
Line 239 assumes
'/'separators. Combined with Line 273 (join(...)) on Windows, barrel resolution can generate bad specifiers and undercount consumers.Suggested fix
function getRelativeJsImport(fromFile, toFile) { - const fromDirectory = fromFile.slice(0, fromFile.lastIndexOf('/')); - const withoutExtension = toFile.replace(/\.tsx?$/, '.js'); + const normalizedFrom = normalizePath(fromFile); + const normalizedTo = normalizePath(toFile); + const fromDirectory = dirname(normalizedFrom); + const withoutExtension = normalizedTo.replace(/\.tsx?$/, '.js'); let specifier = normalizePath(relative(fromDirectory, withoutExtension)); @@ function isExportedFromInternalBarrel(rootDir, filename, className) { - const indexFile = join(rootDir, 'src/internal/index.ts'); + const indexFile = normalizePath(join(rootDir, 'src/internal/index.ts')); if (!existsSync(indexFile)) { return false; }#!/bin/bash # Show the relevant code locations rg -n "function getRelativeJsImport|fromFile\.slice|isExportedFromInternalBarrel|const indexFile = join" projects/internals/eslint/src/local/no-single-consumer-internal-base.js # Demonstrate separator assumption failure python - <<'PY' from pathlib import PureWindowsPath from_file = str(PureWindowsPath(r"C:\repo\pkg\src\internal\index.ts")) print("from_file:", from_file) print("lastIndexOf('/') equivalent:", from_file.rfind('/')) print("derived_from_directory_with_slice:", repr(from_file[:from_file.rfind('/')])) PYAlso applies to: 273-279
🤖 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 `@projects/internals/eslint/src/local/no-single-consumer-internal-base.js` around lines 238 - 241, getRelativeJsImport assumes POSIX separators by slicing fromFile with '/' and later uses join(...) (see indexFile and isExportedFromInternalBarrel usage), which breaks on Windows; before deriving fromDirectory and before calling relative/normalizePath, normalize both fromFile and toFile to use POSIX-style separators (e.g. convert backslashes to '/') or use path.dirname/path.posix functions so normalizePath(relative(...)) receives consistent separators; update references in getRelativeJsImport and the related barrel-resolution code paths (indexFile, isExportedFromInternalBarrel) to operate on the normalized paths.
🤖 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 `@projects/internals/eslint/README.md`:
- Line 77: The README's ESLint rules catalog is missing entries for the two
newly registered rules; add concise entries for no-host-managed-aria-attributes
and no-single-consumer-internal-base to the README: document
no-host-managed-aria-attributes under the "Component API shape" section
(describe that it forbids components from managing ARIA attributes on host that
should be managed by consumers) and document no-single-consumer-internal-base
under "Source hygiene" (describe that it prevents creating internal base classes
that are intended for a single consumer), and ensure the rule names match the
registrations in src/configs/lit.js so readers can locate them.
---
Duplicate comments:
In `@projects/internals/eslint/src/local/no-single-consumer-internal-base.js`:
- Around line 238-241: getRelativeJsImport assumes POSIX separators by slicing
fromFile with '/' and later uses join(...) (see indexFile and
isExportedFromInternalBarrel usage), which breaks on Windows; before deriving
fromDirectory and before calling relative/normalizePath, normalize both fromFile
and toFile to use POSIX-style separators (e.g. convert backslashes to '/') or
use path.dirname/path.posix functions so normalizePath(relative(...)) receives
consistent separators; update references in getRelativeJsImport and the related
barrel-resolution code paths (indexFile, isExportedFromInternalBarrel) to
operate on the normalized paths.
In
`@projects/internals/eslint/src/local/no-single-consumer-internal-base.test.js`:
- Line 5: The test constructs a parent directory by slicing the string filename
with '/' which is not cross-platform; replace the manual slice (occurrences of
filename.slice(0, filename.lastIndexOf('/')) and any similar uses around the
filename variable) with a proper path dirname call (e.g., path.dirname(filename)
or import { dirname } from 'path' and use dirname(filename)) so Windows
backslashes are handled correctly and temp file setup becomes portable; update
any related uses in the same block (lines ~275-279) to use the dirname-based
approach and remove the brittle string slicing.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b5a1ad67-242c-4b9a-b0cc-7b429d59aca6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
knip.config.jsprojects/internals/eslint/README.mdprojects/internals/eslint/src/configs/lit.jsprojects/internals/eslint/src/configs/typescript.jsprojects/internals/eslint/src/local/no-deep-class-inheritance.jsprojects/internals/eslint/src/local/no-deep-class-inheritance.test.jsprojects/internals/eslint/src/local/no-host-managed-aria-attributes.jsprojects/internals/eslint/src/local/no-host-managed-aria-attributes.test.jsprojects/internals/eslint/src/local/no-single-consumer-internal-base.jsprojects/internals/eslint/src/local/no-single-consumer-internal-base.test.jsprojects/site/eleventy.config.jsprojects/site/package.jsonprojects/site/src/_11ty/layouts/docs.11ty.jsprojects/site/src/_11ty/templates/api.jsprojects/site/src/docs/elements/_tabs/examples.11ty.jsprojects/starters/eleventy-ssr/src/index.11ty.js
| **Source hygiene** | ||
|
|
||
| - **`no-dead-code`**. Flags commented-out imports, exports, declarations, control-flow, and test blocks. The project currently sets this to `warn` during cleanup. | ||
| - **`no-deep-class-inheritance`**. Limits class inheritance chains to two superclass hops by default, stopping at configured `allowedRoots` such as `HTMLElement` and `LitElement`. |
There was a problem hiding this comment.
Document the other two new rules.
The PR adds three new rules (no-deep-class-inheritance, no-host-managed-aria-attributes, and no-single-consumer-internal-base), but the README only documents no-deep-class-inheritance. The other two rules are registered in src/configs/lit.js but are missing from this catalog.
Please add entries for:
no-host-managed-aria-attributes(likely under "Component API shape")no-single-consumer-internal-base(likely under "Source hygiene")
🤖 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 `@projects/internals/eslint/README.md` at line 77, The README's ESLint rules
catalog is missing entries for the two newly registered rules; add concise
entries for no-host-managed-aria-attributes and no-single-consumer-internal-base
to the README: document no-host-managed-aria-attributes under the "Component API
shape" section (describe that it forbids components from managing ARIA
attributes on host that should be managed by consumers) and document
no-single-consumer-internal-base under "Source hygiene" (describe that it
prevents creating internal base classes that are intended for a single
consumer), and ensure the rule names match the registrations in
src/configs/lit.js so readers can locate them.
Collaborator
Author
|
🎉 This issue has been resolved in version 0.0.11 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.1.2 🎉 |
Collaborator
Author
|
🎉 This issue has been resolved in version 0.2.0 🎉 |
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
2 participants
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.
Summary by CodeRabbit
New Features
Chores
Chores / Maintenance