feat(lint): improve deprecated api migration rules#58
Conversation
📝 WalkthroughWalkthroughReplaced empty Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/styles/src/responsive.examples.ts (1)
219-269:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent color attribute across similar logo instances.
Line 219 includes
color="brand-green"while the structurally identical logos on lines 229, 239, 249, 259, and 269 lack any color attribute. Since all six logos serve the same role within similar card contexts, they should have consistent styling.🎨 Proposed fix to apply consistent color
- <nve-logo size="lg" nve-display="&sm|hide">NV</nve-logo> + <nve-logo size="lg" nve-display="&sm|hide" color="brand-green">NV</nve-logo>Apply this change to lines 229, 239, 249, 259, and 269.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/styles/src/responsive.examples.ts` around lines 219 - 269, The nve-logo instances are inconsistent: the first nve-logo has color="brand-green" but the repeated nve-logo elements (the ones inside the subsequent nve-card blocks) lack a color attribute; update each of those nve-logo tags to include color="brand-green" so all six logos use the same styling (search for the other <nve-logo size="lg" nve-display="&sm|hide"> occurrences and add the color="brand-green" attribute).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/lint/NOTICE.md`:
- Around line 23-25: Add the new technical term "minisearch" to the Vale
vocabulary allowlist by inserting it into
config/vale/styles/config/vocabularies/Elements/accept.txt in alphabetical
order; update the accept.txt file (so that "minisearch" appears in the proper
alphabetical position among existing entries) to ensure Vale prose linting
recognizes the term referenced in NOTICE.md.
In `@projects/lint/src/eslint/rules/no-deprecated-packages.ts`:
- Around line 46-48: Replace the use of the `in` operator for checking
deprecated packages with an own-property check to avoid inherited properties: in
the filter that builds `deprecatedDependencies` (the expression using
`dependency in DEPRECATED_PACKAGES`), change the predicate to use an
own-property test such as
`Object.prototype.hasOwnProperty.call(DEPRECATED_PACKAGES, dependency)` (or
`Object.hasOwn(DEPRECATED_PACKAGES, dependency)` if your environment supports
it) so only direct keys on `DEPRECATED_PACKAGES` are matched.
---
Outside diff comments:
In `@projects/styles/src/responsive.examples.ts`:
- Around line 219-269: The nve-logo instances are inconsistent: the first
nve-logo has color="brand-green" but the repeated nve-logo elements (the ones
inside the subsequent nve-card blocks) lack a color attribute; update each of
those nve-logo tags to include color="brand-green" so all six logos use the same
styling (search for the other <nve-logo size="lg" nve-display="&sm|hide">
occurrences and add the color="brand-green" attribute).
🪄 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: 3e7d852c-d864-4d2b-b0eb-fb1fd92f15e9
📒 Files selected for processing (32)
NOTICE.mdprojects/core/src/logo/logo.examples.tsprojects/core/src/logo/logo.test.axe.tsprojects/core/src/logo/logo.test.lighthouse.tsprojects/core/src/logo/logo.test.ssr.tsprojects/core/src/logo/logo.test.tsprojects/core/src/menu/menu.examples.tsprojects/core/src/page-header/page-header.snippets.htmlprojects/core/src/page/page.test.visual.tsprojects/core/src/preferences-input/preferences-input.examples.tsprojects/lint/NOTICE.mdprojects/lint/src/eslint/rules/no-deprecated-css-imports.test.tsprojects/lint/src/eslint/rules/no-deprecated-css-imports.tsprojects/lint/src/eslint/rules/no-deprecated-packages.test.tsprojects/lint/src/eslint/rules/no-deprecated-packages.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.test.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.tsprojects/site/src/_internal/animated-build/animated-build.tsprojects/site/src/_internal/stories/theme/theme-generator.tsprojects/starters/angular/src/app/app.htmlprojects/starters/bundles/src/index.htmlprojects/starters/eleventy/src/_layouts/index.11ty.jsprojects/starters/go/src/index.htmlprojects/starters/hugo/layouts/baseof.htmlprojects/starters/mpa/src/about/index.htmlprojects/starters/mpa/src/index.htmlprojects/starters/mpa/src/settings/index.htmlprojects/starters/nextjs/src/pages/index.tsxprojects/starters/react/src/App.tsxprojects/starters/solidjs/src/App.tsxprojects/starters/typescript/src/index.htmlprojects/styles/src/responsive.examples.ts
05fe508 to
faa042e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
projects/lint/src/eslint/rules/no-deprecated-packages.ts (1)
46-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse own-property lookup instead of prototype-chain membership.
Line 47 uses
dependency in DEPRECATED_PACKAGES, which can match inherited keys and produce false positives (andundefinedalternatives in Line 56). Use an own-property check.Suggested fix
- const deprecatedDependencies = [...dependencies, ...devDependencies, ...peerDependencies].filter( - dependency => dependency in DEPRECATED_PACKAGES - ); + const deprecatedDependencies = [...dependencies, ...devDependencies, ...peerDependencies].filter( + dependency => Object.prototype.hasOwnProperty.call(DEPRECATED_PACKAGES, dependency) + );#!/bin/bash # Verify the rule still uses prototype-chain membership instead of own-property checks. rg -nP --type=ts -C2 \ 'dependency\s*=>\s*dependency in DEPRECATED_PACKAGES|hasOwnProperty\.call\(DEPRECATED_PACKAGES, dependency\)|Object\.hasOwn\(DEPRECATED_PACKAGES, dependency\)' \ projects/lint/src/eslint/rules/no-deprecated-packages.tsExpected: current match shows
dependency in DEPRECATED_PACKAGES; after fix it should show an own-property check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/lint/src/eslint/rules/no-deprecated-packages.ts` around lines 46 - 48, The filter is using prototype-chain membership ("dependency in DEPRECATED_PACKAGES") which can yield inherited keys; change it to an own-property check so deprecatedDependencies only contains true keys of DEPRECATED_PACKAGES—use Object.hasOwn(DEPRECATED_PACKAGES, dependency) or Object.prototype.hasOwnProperty.call(DEPRECATED_PACKAGES, dependency) in the filter that builds deprecatedDependencies (refer to the constant DEPRECATED_PACKAGES and the variable dependency in that filter).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts`:
- Around line 146-160: Add an extra invalid test case to the logos group that
covers a whitespace-only default slot to prevent blank text nodes from being
treated as valid; inside the tester.run call for the 'logos missing default slot
content' test (the one using rule and errors with messageId
'missing-default-slot-content' and data { tagName: 'nve-logo' }), add another
invalid entry whose code is an nve-logo containing only whitespace (e.g.,
`<nve-logo> </nve-logo>`) and the same expected errors array so the rule rejects
whitespace-only default content.
In `@projects/site/src/_internal/animated-build/animated-build.ts`:
- Line 231: The nve-logo element used in the animated build template (<nve-logo
slot="prefix" size="sm">NV</nve-logo>) is missing the color="brand-green"
attribute that's used elsewhere; either add color="brand-green" to this nve-logo
to match the established pattern or, if the omission is intentional for the
animation, add a brief inline comment next to the nve-logo element (or above it
in the AnimatedBuild template) stating that the color is omitted deliberately to
preserve the demo styling.
In `@projects/starters/angular/src/app/app.html`:
- Line 3: The header logo element <nve-logo> should include an accessible name
for assistive tech; update the <nve-logo> usage (the element with slot="prefix"
size="sm" color="brand-green") to add an aria-label attribute such as
aria-label="NVIDIA" (or aria-label="NVIDIA logo") so screen readers get a clear,
descriptive name for the brand logo.
---
Duplicate comments:
In `@projects/lint/src/eslint/rules/no-deprecated-packages.ts`:
- Around line 46-48: The filter is using prototype-chain membership ("dependency
in DEPRECATED_PACKAGES") which can yield inherited keys; change it to an
own-property check so deprecatedDependencies only contains true keys of
DEPRECATED_PACKAGES—use Object.hasOwn(DEPRECATED_PACKAGES, dependency) or
Object.prototype.hasOwnProperty.call(DEPRECATED_PACKAGES, dependency) in the
filter that builds deprecatedDependencies (refer to the constant
DEPRECATED_PACKAGES and the variable dependency in that filter).
🪄 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: af79ecef-e7bd-47e4-b668-62cc1fcb9492
📒 Files selected for processing (32)
NOTICE.mdprojects/core/src/logo/logo.examples.tsprojects/core/src/logo/logo.test.axe.tsprojects/core/src/logo/logo.test.lighthouse.tsprojects/core/src/logo/logo.test.ssr.tsprojects/core/src/logo/logo.test.tsprojects/core/src/menu/menu.examples.tsprojects/core/src/page-header/page-header.snippets.htmlprojects/core/src/page/page.test.visual.tsprojects/core/src/preferences-input/preferences-input.examples.tsprojects/lint/NOTICE.mdprojects/lint/src/eslint/rules/no-deprecated-css-imports.test.tsprojects/lint/src/eslint/rules/no-deprecated-css-imports.tsprojects/lint/src/eslint/rules/no-deprecated-packages.test.tsprojects/lint/src/eslint/rules/no-deprecated-packages.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.test.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.tsprojects/site/src/_internal/animated-build/animated-build.tsprojects/site/src/_internal/stories/theme/theme-generator.tsprojects/starters/angular/src/app/app.htmlprojects/starters/bundles/src/index.htmlprojects/starters/eleventy/src/_layouts/index.11ty.jsprojects/starters/go/src/index.htmlprojects/starters/hugo/layouts/baseof.htmlprojects/starters/mpa/src/about/index.htmlprojects/starters/mpa/src/index.htmlprojects/starters/mpa/src/settings/index.htmlprojects/starters/nextjs/src/pages/index.tsxprojects/starters/react/src/App.tsxprojects/starters/solidjs/src/App.tsxprojects/starters/typescript/src/index.htmlprojects/styles/src/responsive.examples.ts
| it('should report logos missing default slot content', () => { | ||
| tester.run('logos missing default slot content', rule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `<nve-logo></nve-logo>`, | ||
| errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] | ||
| }, | ||
| { | ||
| code: `<nve-logo><span slot="prefix">NV</span></nve-logo>`, | ||
| errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] | ||
| } | ||
| ] | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add a whitespace-only nve-logo invalid case to lock down an edge condition.
You already enforce this for nve-tree-node; adding the same assertion for nve-logo prevents regressions where blank text nodes are treated as valid content.
Proposed test diff
it('should report logos missing default slot content', () => {
tester.run('logos missing default slot content', rule, {
valid: [],
invalid: [
{
code: `<nve-logo></nve-logo>`,
errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }]
},
+ {
+ code: `<nve-logo> </nve-logo>`,
+ errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }]
+ },
{
code: `<nve-logo><span slot="prefix">NV</span></nve-logo>`,
errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }]
}
]
});
});📝 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.
| it('should report logos missing default slot content', () => { | |
| tester.run('logos missing default slot content', rule, { | |
| valid: [], | |
| invalid: [ | |
| { | |
| code: `<nve-logo></nve-logo>`, | |
| errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] | |
| }, | |
| { | |
| code: `<nve-logo><span slot="prefix">NV</span></nve-logo>`, | |
| errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] | |
| } | |
| ] | |
| }); | |
| }); | |
| it('should report logos missing default slot content', () => { | |
| tester.run('logos missing default slot content', rule, { | |
| valid: [], | |
| invalid: [ | |
| { | |
| code: `<nve-logo></nve-logo>`, | |
| errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] | |
| }, | |
| { | |
| code: `<nve-logo> </nve-logo>`, | |
| errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] | |
| }, | |
| { | |
| code: `<nve-logo><span slot="prefix">NV</span></nve-logo>`, | |
| errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] | |
| } | |
| ] | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts` around
lines 146 - 160, Add an extra invalid test case to the logos group that covers a
whitespace-only default slot to prevent blank text nodes from being treated as
valid; inside the tester.run call for the 'logos missing default slot content'
test (the one using rule and errors with messageId
'missing-default-slot-content' and data { tagName: 'nve-logo' }), add another
invalid entry whose code is an nve-logo containing only whitespace (e.g.,
`<nve-logo> </nve-logo>`) and the same expected errors array so the rule rejects
whitespace-only default content.
| <nve-page class="layer"> | ||
| <nve-page-header slot="header" class="layer" style="--delay: 0.7s"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm">NV</nve-logo> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other nve-logo instances in the codebase use color="brand-green"
rg -n -C2 'nve-logo.*color="brand-green"' --type ts --type htmlRepository: NVIDIA/elements
Length of output: 48368
🏁 Script executed:
cat -n projects/site/src/_internal/animated-build/animated-build.ts | sed -n '220,240p'Repository: NVIDIA/elements
Length of output: 1454
🏁 Script executed:
rg -n 'nve-logo' projects/site/src/_internal/animated-build/animated-build.tsRepository: NVIDIA/elements
Length of output: 127
Consider adding color="brand-green" for consistency with other nve-logo instances.
The nve-logo at line 231 omits the color="brand-green" attribute present in 50+ instances across the codebase. While this may be intentional for the animated demo styling, adding the attribute would align with the established pattern. If the omission is deliberate to maintain a specific visual treatment in the animation, a comment explaining this choice would be helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/site/src/_internal/animated-build/animated-build.ts` at line 231,
The nve-logo element used in the animated build template (<nve-logo
slot="prefix" size="sm">NV</nve-logo>) is missing the color="brand-green"
attribute that's used elsewhere; either add color="brand-green" to this nve-logo
to match the established pattern or, if the omission is intentional for the
animation, add a brief inline comment next to the nve-logo element (or above it
in the AnimatedBuild template) stating that the color is omitted deliberately to
preserve the demo styling.
| <nve-page> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> |
There was a problem hiding this comment.
Add a descriptive accessible name to the header logo.
Using NV as visible text is fine visually, but adding aria-label="NVIDIA" makes the brand intent explicit for assistive tech.
Suggested fix
- <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/starters/angular/src/app/app.html` at line 3, The header logo
element <nve-logo> should include an accessible name for assistive tech; update
the <nve-logo> usage (the element with slot="prefix" size="sm"
color="brand-green") to add an aria-label attribute such as aria-label="NVIDIA"
(or aria-label="NVIDIA logo") so screen readers get a clear, descriptive name
for the brand logo.
faa042e to
e51f1be
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts (1)
146-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the whitespace-only
nve-logoinvalid case.The logos suite still misses the blank-text regression that you already guard for on
nve-tree-node; a whitespace-only<nve-logo>should be rejected as well.Proposed test addition
{ code: `<nve-logo><span slot="prefix">NV</span></nve-logo>`, errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] }, + { + code: `<nve-logo> </nve-logo>`, + errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts` around lines 146 - 160, Add an invalid test case that rejects a whitespace-only <nve-logo> node: in the logos test block (the tester.run call for "logos missing default slot content") include an additional invalid entry where code is a <nve-logo> containing only whitespace (e.g., space or newline) and the expected error matches messageId 'missing-default-slot-content' with data { tagName: 'nve-logo' }; update the invalid array alongside the existing empty and slot-only cases so the rule (referenced as rule in this test) will catch blank-text content the same way it does for nve-tree-node.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/lint/src/eslint/rules/no-deprecated-css-imports.test.ts`:
- Around line 57-62: Add an assertion to the test "should cover CSS entrypoints
documented in the migration guide" to also verify the shared mapping for the
core entrypoint: check that deprecatedImports['@nvidia-elements/core/index.css']
contains the same string as the maglev entrypoint (i.e. the view-transitions
import) so the reused template cannot drift; update the test near the existing
expects that reference deprecatedImports and the view-transitions import string.
In `@projects/lint/src/eslint/rules/no-deprecated-packages.test.ts`:
- Around line 105-148: Add a new invalid test case to the tester.run call in
no-deprecated-packages.test.ts that simulates the same deprecated package
appearing in multiple sections (e.g., dependencies and devDependencies or
dependencies and peerDependencies) so the rule reports both occurrences;
specifically add an entry to the invalid array used by tester.run (alongside the
existing objects) with code containing the duplicate package (for example both
"dependencies" and "devDependencies" containing "@nve/elements") and an errors
array expecting two 'unexpected-deprecated-package' diagnostics referencing the
same package and appropriate alternative text to ensure duplicate reporting is
covered by the test.
In `@projects/lint/src/eslint/rules/no-deprecated-packages.ts`:
- Around line 46-59: The current logic builds deprecatedDependencies by
concatenating dependencies, devDependencies, and peerDependencies and then
reports each entry, which causes duplicate diagnostics when a package appears in
multiple sections; change the construction of deprecatedDependencies to
deduplicate package names (e.g., build a Set or use Array.from(new Set(...))
over [...dependencies, ...devDependencies, ...peerDependencies].filter(...)) so
each package key present in DEPRECATED_PACKAGES is reported only once, and
continue to call context.report with loc: node.loc! and data using
DEPRECATED_PACKAGES[dependency].
---
Duplicate comments:
In `@projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts`:
- Around line 146-160: Add an invalid test case that rejects a whitespace-only
<nve-logo> node: in the logos test block (the tester.run call for "logos missing
default slot content") include an additional invalid entry where code is a
<nve-logo> containing only whitespace (e.g., space or newline) and the expected
error matches messageId 'missing-default-slot-content' with data { tagName:
'nve-logo' }; update the invalid array alongside the existing empty and
slot-only cases so the rule (referenced as rule in this test) will catch
blank-text content the same way it does for nve-tree-node.
🪄 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: 3425430a-2e4e-46d0-82ff-8349e0e9d5e3
📒 Files selected for processing (30)
projects/core/src/logo/logo.examples.tsprojects/core/src/logo/logo.test.axe.tsprojects/core/src/logo/logo.test.lighthouse.tsprojects/core/src/logo/logo.test.ssr.tsprojects/core/src/logo/logo.test.tsprojects/core/src/menu/menu.examples.tsprojects/core/src/page-header/page-header.snippets.htmlprojects/core/src/page/page.test.visual.tsprojects/core/src/preferences-input/preferences-input.examples.tsprojects/lint/src/eslint/rules/no-deprecated-css-imports.test.tsprojects/lint/src/eslint/rules/no-deprecated-css-imports.tsprojects/lint/src/eslint/rules/no-deprecated-packages.test.tsprojects/lint/src/eslint/rules/no-deprecated-packages.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.test.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.tsprojects/site/src/_internal/animated-build/animated-build.tsprojects/site/src/_internal/stories/theme/theme-generator.tsprojects/starters/angular/src/app/app.htmlprojects/starters/bundles/src/index.htmlprojects/starters/eleventy/src/_layouts/index.11ty.jsprojects/starters/go/src/index.htmlprojects/starters/hugo/layouts/baseof.htmlprojects/starters/mpa/src/about/index.htmlprojects/starters/mpa/src/index.htmlprojects/starters/mpa/src/settings/index.htmlprojects/starters/nextjs/src/pages/index.tsxprojects/starters/react/src/App.tsxprojects/starters/solidjs/src/App.tsxprojects/starters/typescript/src/index.htmlprojects/styles/src/responsive.examples.ts
- New deprecated CSS imports in the no-deprecated-css-imports rule - Updated the no-deprecated-packages - Updated no-missing-slotted-elements for nve-logo Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
e51f1be to
ecfab75
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
projects/lint/src/eslint/rules/no-deprecated-packages.ts (1)
46-48:⚠️ Potential issue | 🟠 MajorUse an own-property check for deprecated package lookup.
Line 47can match inherited keys liketoStringandconstructor, so non-deprecated packages with those names would be falsely reported andDEPRECATED_PACKAGES[dependency]would not be a valid upgrade target. This was raised earlier and still appears in the current revision.Suggested fix
- const deprecatedDependencies = [...dependencies, ...devDependencies, ...peerDependencies].filter( - dependency => dependency in DEPRECATED_PACKAGES - ); + const deprecatedDependencies = [...dependencies, ...devDependencies, ...peerDependencies].filter( + dependency => Object.prototype.hasOwnProperty.call(DEPRECATED_PACKAGES, dependency) + );Verify the current predicate and the language behavior with:
#!/bin/bash sed -n '46,48p' projects/lint/src/eslint/rules/no-deprecated-packages.ts node - <<'NODE' const deprecated = { '@nve/elements': '@nvidia-elements/core' }; for (const dep of ['toString', 'constructor', '@nve/elements']) { console.log( dep, 'in =>', dep in deprecated, 'hasOwn =>', Object.prototype.hasOwnProperty.call(deprecated, dep) ); } NODEExpected result:
toStringandconstructorprintin => truebuthasOwn => false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/lint/src/eslint/rules/no-deprecated-packages.ts` around lines 46 - 48, The filter currently uses the "in" operator which matches inherited keys and can falsely mark names like "toString" or "constructor"; update the predicate in the creation of deprecatedDependencies (the array built from dependencies, devDependencies, peerDependencies) to use Object.prototype.hasOwnProperty.call(DEPRECATED_PACKAGES, dependency) (or an equivalent own-property check) so only actual keys on DEPRECATED_PACKAGES are considered deprecated and DEPRECATED_PACKAGES[dependency] is then guaranteed to be a valid upgrade target.projects/starters/angular/src/app/app.html (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd aria-label for assistive technology.
The
nve-logoelement would benefit from an explicit accessible name likearia-label="NVIDIA"to provide clear brand identification for screen readers, as previously noted.♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> + <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/starters/angular/src/app/app.html` at line 3, The custom element <nve-logo> lacks an accessible name for screen readers; add an explicit aria-label (for example aria-label="NVIDIA") to the nve-logo tag (the element with attributes slot="prefix" size="sm" color="brand-green") so assistive technology can identify the brand.projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts (1)
146-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd whitespace-only
nve-logoinvalid coverage.Line 146 test group still misses the whitespace-only case (
<nve-logo> </nve-logo>), so this edge condition can regress even thoughhasUnslottedContent()trims text.Proposed test diff
it('should report logos missing default slot content', () => { tester.run('logos missing default slot content', rule, { valid: [], invalid: [ { code: `<nve-logo></nve-logo>`, errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] }, + { + code: `<nve-logo> </nve-logo>`, + errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] + }, { code: `<nve-logo><span slot="prefix">NV</span></nve-logo>`, errors: [{ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' } }] } ] }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts` around lines 146 - 160, Add an invalid test case that covers whitespace-only default slot content so the rule flags `<nve-logo> </nve-logo>` as missing content; update the `it('should report logos missing default slot content'...)` test in no-missing-slotted-elements.test.ts to include an additional invalid entry with code `<nve-logo> </nve-logo>` and the same errors payload ({ messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' }}) to ensure the rule (and the helper hasUnslottedContent()) treats trimmed-only text as missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/src/page-header/page-header.snippets.html`:
- Line 3: Add an aria-label to the logo snippet so the pattern promotes
accessibility: update the <nve-logo> element (the snippet containing <nve-logo
slot="prefix" size="sm">NV</nve-logo>) to include aria-label="NVIDIA" so
assistive technologies have a readable name; ensure the attribute is added
directly on the nve-logo tag in the page-header snippet template.
- Line 14: The snippet's nve-logo element (the <nve-logo slot="prefix"
size="sm">NV</nve-logo> instance) lacks an accessible name—add an aria-label
attribute (e.g., aria-label="NVIDIA") to the nve-logo element so screen readers
announce the logo; update the snippet template where nve-logo is used
(slot="prefix", size="sm") to include aria-label="NVIDIA" to establish the
accessible pattern for consumers.
In `@projects/site/src/_internal/stories/theme/theme-generator.ts`:
- Line 68: The nve-logo element used in the theme generator lacks an accessible
name; update the <nve-logo slot="prefix" size="sm">NV</nve-logo> instance to
include an appropriate aria-label (e.g., aria-label="NVIDIA") so screen readers
receive meaningful context for the logo component.
In `@projects/starters/eleventy/src/_layouts/index.11ty.js`:
- Line 19: Add an accessible label to the custom element by adding
aria-label="NVIDIA" to the nve-logo element (nve-logo slot="prefix" size="sm"
color="brand-green") so screen readers announce the brand name explicitly;
update the nve-logo tag to include aria-label="NVIDIA".
In `@projects/starters/mpa/src/settings/index.html`:
- Line 15: The nve-logo element is missing an accessible name for screen
readers; update the <nve-logo> element (the nve-logo tag with slot="prefix"
size="sm" color="brand-green") to include an aria-label attribute, e.g.,
aria-label="NVIDIA", so assistive technologies read the brand name instead of
only the visual "NV".
In `@projects/starters/nextjs/src/pages/index.tsx`:
- Line 15: The nve-logo element lacks an accessibility label; update the
<nve-logo> element in pages/index.tsx (the nve-logo component instance) to
include aria-label="NVIDIA" so screen readers receive the full brand name;
ensure the new aria-label attribute is added directly on the nve-logo tag
alongside existing attributes (slot, size, color) to preserve visual behavior.
In `@projects/starters/solidjs/src/App.tsx`:
- Line 13: Add an aria-label to the custom element used for the logo so screen
readers get a meaningful name: update the <nve-logo> element in App.tsx (the
element with slot="prefix" size="sm" color="brand-green") to include
aria-label="NVIDIA" to improve accessibility for assistive technologies.
In `@projects/starters/typescript/src/index.html`:
- Line 13: The nve-logo element lacks an accessible name for assistive tech;
update the <nve-logo> instance (the element with slot="prefix" and content "NV")
to include an aria-label attribute set to "NVIDIA" so screen readers receive
explicit brand identification (i.e., add aria-label="NVIDIA" to that nve-logo
element).
---
Duplicate comments:
In `@projects/lint/src/eslint/rules/no-deprecated-packages.ts`:
- Around line 46-48: The filter currently uses the "in" operator which matches
inherited keys and can falsely mark names like "toString" or "constructor";
update the predicate in the creation of deprecatedDependencies (the array built
from dependencies, devDependencies, peerDependencies) to use
Object.prototype.hasOwnProperty.call(DEPRECATED_PACKAGES, dependency) (or an
equivalent own-property check) so only actual keys on DEPRECATED_PACKAGES are
considered deprecated and DEPRECATED_PACKAGES[dependency] is then guaranteed to
be a valid upgrade target.
In `@projects/lint/src/eslint/rules/no-missing-slotted-elements.test.ts`:
- Around line 146-160: Add an invalid test case that covers whitespace-only
default slot content so the rule flags `<nve-logo> </nve-logo>` as missing
content; update the `it('should report logos missing default slot content'...)`
test in no-missing-slotted-elements.test.ts to include an additional invalid
entry with code `<nve-logo> </nve-logo>` and the same errors payload ({
messageId: 'missing-default-slot-content', data: { tagName: 'nve-logo' }}) to
ensure the rule (and the helper hasUnslottedContent()) treats trimmed-only text
as missing.
In `@projects/starters/angular/src/app/app.html`:
- Line 3: The custom element <nve-logo> lacks an accessible name for screen
readers; add an explicit aria-label (for example aria-label="NVIDIA") to the
nve-logo tag (the element with attributes slot="prefix" size="sm"
color="brand-green") so assistive technology can identify the brand.
🪄 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: ca2fc5e9-d40b-47c6-88f4-dfed9d0323bb
📒 Files selected for processing (30)
projects/core/src/logo/logo.examples.tsprojects/core/src/logo/logo.test.axe.tsprojects/core/src/logo/logo.test.lighthouse.tsprojects/core/src/logo/logo.test.ssr.tsprojects/core/src/logo/logo.test.tsprojects/core/src/menu/menu.examples.tsprojects/core/src/page-header/page-header.snippets.htmlprojects/core/src/page/page.test.visual.tsprojects/core/src/preferences-input/preferences-input.examples.tsprojects/lint/src/eslint/rules/no-deprecated-css-imports.test.tsprojects/lint/src/eslint/rules/no-deprecated-css-imports.tsprojects/lint/src/eslint/rules/no-deprecated-packages.test.tsprojects/lint/src/eslint/rules/no-deprecated-packages.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.test.tsprojects/lint/src/eslint/rules/no-missing-slotted-elements.tsprojects/site/src/_internal/animated-build/animated-build.tsprojects/site/src/_internal/stories/theme/theme-generator.tsprojects/starters/angular/src/app/app.htmlprojects/starters/bundles/src/index.htmlprojects/starters/eleventy/src/_layouts/index.11ty.jsprojects/starters/go/src/index.htmlprojects/starters/hugo/layouts/baseof.htmlprojects/starters/mpa/src/about/index.htmlprojects/starters/mpa/src/index.htmlprojects/starters/mpa/src/settings/index.htmlprojects/starters/nextjs/src/pages/index.tsxprojects/starters/react/src/App.tsxprojects/starters/solidjs/src/App.tsxprojects/starters/typescript/src/index.htmlprojects/styles/src/responsive.examples.ts
| <template name="Page Header" prefix="page-header" description="Basic page-header layout." type="pattern"> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm">NV</nve-logo> |
There was a problem hiding this comment.
Consider adding aria-label to the snippet template.
Since these snippets establish patterns for developers, including aria-label="NVIDIA" would promote accessible implementations.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/src/page-header/page-header.snippets.html` at line 3, Add an
aria-label to the logo snippet so the pattern promotes accessibility: update the
<nve-logo> element (the snippet containing <nve-logo slot="prefix"
size="sm">NV</nve-logo>) to include aria-label="NVIDIA" so assistive
technologies have a readable name; ensure the attribute is added directly on the
nve-logo tag in the page-header snippet template.
| type="pattern"> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm">NV</nve-logo> |
There was a problem hiding this comment.
Consider adding aria-label to the snippet template.
Since these snippets establish patterns for developers, including aria-label="NVIDIA" would promote accessible implementations.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/src/page-header/page-header.snippets.html` at line 14, The
snippet's nve-logo element (the <nve-logo slot="prefix" size="sm">NV</nve-logo>
instance) lacks an accessible name—add an aria-label attribute (e.g.,
aria-label="NVIDIA") to the nve-logo element so screen readers announce the
logo; update the snippet template where nve-logo is used (slot="prefix",
size="sm") to include aria-label="NVIDIA" to establish the accessible pattern
for consumers.
| <nve-page> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm">NV</nve-logo> |
There was a problem hiding this comment.
Add aria-label for accessibility.
Even in demo components, including aria-label="NVIDIA" provides better screen reader context.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/site/src/_internal/stories/theme/theme-generator.ts` at line 68, The
nve-logo element used in the theme generator lacks an accessible name; update
the <nve-logo slot="prefix" size="sm">NV</nve-logo> instance to include an
appropriate aria-label (e.g., aria-label="NVIDIA") so screen readers receive
meaningful context for the logo component.
| <nve-page> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> |
There was a problem hiding this comment.
Add aria-label for screen reader users.
Include aria-label="NVIDIA" to provide explicit brand identification for assistive technology.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/starters/eleventy/src/_layouts/index.11ty.js` at line 19, Add an
accessible label to the custom element by adding aria-label="NVIDIA" to the
nve-logo element (nve-logo slot="prefix" size="sm" color="brand-green") so
screen readers announce the brand name explicitly; update the nve-logo tag to
include aria-label="NVIDIA".
| <nve-page view-transition> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> |
There was a problem hiding this comment.
Add aria-label for better screen reader context.
While "NV" provides visual content, an aria-label="NVIDIA" attribute would give assistive technology users clearer brand identification.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/starters/mpa/src/settings/index.html` at line 15, The nve-logo
element is missing an accessible name for screen readers; update the <nve-logo>
element (the nve-logo tag with slot="prefix" size="sm" color="brand-green") to
include an aria-label attribute, e.g., aria-label="NVIDIA", so assistive
technologies read the brand name instead of only the visual "NV".
| <nve-page> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> |
There was a problem hiding this comment.
Add aria-label for improved accessibility.
Include aria-label="NVIDIA" to give screen readers explicit brand context beyond the abbreviated visual text.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/starters/nextjs/src/pages/index.tsx` at line 15, The nve-logo
element lacks an accessibility label; update the <nve-logo> element in
pages/index.tsx (the nve-logo component instance) to include aria-label="NVIDIA"
so screen readers receive the full brand name; ensure the new aria-label
attribute is added directly on the nve-logo tag alongside existing attributes
(slot, size, color) to preserve visual behavior.
| <nve-page> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> |
There was a problem hiding this comment.
Add aria-label for screen reader accessibility.
Including aria-label="NVIDIA" would provide clearer semantic meaning for assistive technology users.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/starters/solidjs/src/App.tsx` at line 13, Add an aria-label to the
custom element used for the logo so screen readers get a meaningful name: update
the <nve-logo> element in App.tsx (the element with slot="prefix" size="sm"
color="brand-green") to include aria-label="NVIDIA" to improve accessibility for
assistive technologies.
| <nve-page> | ||
| <nve-page-header slot="header"> | ||
| <nve-logo slot="prefix" size="sm"></nve-logo> | ||
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> |
There was a problem hiding this comment.
Add aria-label for assistive technology.
Include aria-label="NVIDIA" to provide explicit brand identification for screen reader users.
♿ Suggested accessibility improvement
- <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo>
+ <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo>📝 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.
| <nve-logo slot="prefix" size="sm" color="brand-green">NV</nve-logo> | |
| <nve-logo slot="prefix" size="sm" color="brand-green" aria-label="NVIDIA">NV</nve-logo> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/starters/typescript/src/index.html` at line 13, The nve-logo element
lacks an accessible name for assistive tech; update the <nve-logo> instance (the
element with slot="prefix" and content "NV") to include an aria-label attribute
set to "NVIDIA" so screen readers receive explicit brand identification (i.e.,
add aria-label="NVIDIA" to that nve-logo element).
|
🎉 This issue has been resolved in version 0.3.0 🎉 |
|
🎉 This issue has been resolved in version 0.0.10 🎉 |
|
🎉 This issue has been resolved in version 0.1.0 🎉 |
Summary by CodeRabbit
Style
Chores