feat(lint): no deprecated global attribute value#29
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new HTML ESLint rule to detect and autofix deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant Source as "HTML Source"
participant Parser as "HTML Parser"
participant ESLint as "ESLint Core"
participant Rule as "no-deprecated-global-attribute-value"
participant Fixer as "Auto-fix / Suggestion"
Source->>Parser: Parse HTML document
Parser->>ESLint: Emit AST/tokens
ESLint->>Rule: Evaluate elements & attributes
Rule->>Rule: Tokenize attribute values, skip bindings, compute replacements
alt Deprecated tokens found
Rule->>ESLint: Report error + provide fix & suggestion
ESLint->>Fixer: Apply autofix or surface suggestion
Fixer-->>Source: Produce fixed attribute value
else No deprecated tokens
Rule-->>ESLint: No report
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-global-attribute-value.test.ts`:
- Around line 94-117: Add a regression test that ensures repeated non-deprecated
tokens are preserved after autofix: extend the existing tester.run usage for the
rule in no-deprecated-global-attribute-value.test.ts (the same 'multi-token
replacement' scenario) with an additional invalid case where the input contains
repeated tokens (e.g. both occurrences of "grow" repeated), assert the output
preserves repeated tokens after replacement (e.g. both "grow" -> "full" so "grow
grow column" -> "full full column"), and include the corresponding
errors.suggestions.output and errors.data entries so the autofix and suggestion
behavior is validated.
In `@projects/lint/src/eslint/rules/no-deprecated-global-attribute-value.ts`:
- Line 25: The autofix currently uses Array.from(new Set(replaced.flatMap(t =>
t.split(/\s+/)))).join(' ') which deduplicates tokens and changes the user’s
original input; instead, perform token-by-token replacement that preserves
original spacing and token order (do not use Set). Locate the code that builds
the autofix output (the expression using replaced and flatMap) and replace it
with logic that maps over the original attribute string or its token sequence,
replacing only deprecated tokens in-place while keeping duplicates and the
original whitespace/ordering intact (identify this by the variable name replaced
and the current .flatMap(...).join(' ') expression).
In `@projects/site/src/docs/internal/guidelines/component-creation.md`:
- Line 177: The doc contains inconsistent import sources for test utilities:
update the accessibility test snippet that currently imports from
'@nvidia-elements/testing' to use the same package as the unit-test snippet
('@internals/testing'); change the import statement that references
createFixture, elementIsStable, removeFixture (or any of those symbols) so it
imports them from '@internals/testing' instead of '@nvidia-elements/testing' to
avoid copy/paste errors.
🪄 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: fdc090b8-089d-45d4-a3b8-77ae28ec0545
⛔ Files ignored due to path filters (3)
projects/core/.visual/icon.dark.pngis excluded by!**/*.pngprojects/core/.visual/icon.pngis excluded by!**/*.pngprojects/core/src/icon/icons/stop.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
projects/cli/package.jsonprojects/core/src/grid/grid.examples.tsprojects/core/src/icon/icons.tsprojects/core/src/icon/server.tsprojects/core/src/progress-bar/progress-bar.examples.tsprojects/internals/patterns/src/heatmap.examples.tsprojects/internals/tools/src/context/migration.mdprojects/internals/tools/src/project/starters.tsprojects/lint/README.mdprojects/lint/src/eslint/configs/html.tsprojects/lint/src/eslint/rules/no-deprecated-global-attribute-value.test.tsprojects/lint/src/eslint/rules/no-deprecated-global-attribute-value.tsprojects/site/src/_11ty/layouts/docs.11ty.jsprojects/site/src/docs/internal/guidelines/component-creation.mdprojects/site/src/docs/lint/index.mdprojects/site/src/docs/testing/index.md
💤 Files with no reviewable changes (2)
- projects/internals/tools/src/context/migration.md
- projects/site/src/docs/testing/index.md
c22a5c4 to
b961259
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/internals/tools/src/project/starters.ts`:
- Line 118: The code unconditionally writes a project .npmrc via
writeFile(`${dist}/.npmrc`, `registry=${ELEMENTS_REGISTRY_URL}/`) which can
override user/CI registries; change it to only create/modify the file when
necessary: check ELEMENTS_REGISTRY_URL and skip writing if it is falsy or equals
the public npm default (https://registry.npmjs.org/) or if a user/CI registry is
already configured (e.g. process.env.npm_config_registry or an existing
`${dist}/.npmrc` contains a registry entry); update the logic surrounding the
writeFile call so it returns early when none of those conditions require
changing the registry, otherwise write the registry line as before.
- Around line 20-21: Replace the hard-coded constants ELEMENTS_PAGES_BASE_URL
and ELEMENTS_REGISTRY_URL with values read from environment variables (e.g.
process.env.ELEMENTS_PAGES_BASE_URL and process.env.ELEMENTS_REGISTRY_URL) while
falling back to the current URLs as safe defaults; update the definitions in
starters.ts to use the environment values (and optionally trim/normalise them)
so runtime/CI/staging/mirror or air-gapped setups can override endpoints without
changing code.
🪄 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: 7199efed-1512-41e1-9c60-f819ee3231fa
📒 Files selected for processing (10)
projects/cli/package.jsonprojects/core/src/grid/grid.examples.tsprojects/core/src/progress-bar/progress-bar.examples.tsprojects/internals/patterns/src/heatmap.examples.tsprojects/internals/tools/src/context/migration.mdprojects/internals/tools/src/project/starters.tsprojects/site/src/_11ty/layouts/docs.11ty.jsprojects/site/src/docs/internal/guidelines/component-creation.mdprojects/site/src/docs/labs/index.mdprojects/site/src/docs/testing/index.md
💤 Files with no reviewable changes (3)
- projects/site/src/docs/labs/index.md
- projects/internals/tools/src/context/migration.md
- projects/site/src/docs/testing/index.md
b961259 to
7fdb1e3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projects/internals/tools/src/project/starters.ts (1)
20-107:⚠️ Potential issue | 🟠 MajorRestore runtime configurability for
ELEMENTS_PAGES_BASE_URL.Hardcoding the production pages URL removes environment override flexibility (staging/mirror/air-gapped) and makes this module inconsistent with other env-injected URL usage in the toolchain.
Proposed fix
-import { cwd } from 'node:process'; +import { cwd, env } from 'node:process'; @@ -const ELEMENTS_PAGES_BASE_URL = 'https://nvidia.github.io/elements'; +const DEFAULT_ELEMENTS_PAGES_BASE_URL = 'https://nvidia.github.io/elements'; +const ELEMENTS_PAGES_BASE_URL = + env.ELEMENTS_PAGES_BASE_URL?.replace(/\/+$/, '') || DEFAULT_ELEMENTS_PAGES_BASE_URL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/internals/tools/src/project/starters.ts` around lines 20 - 107, The constant ELEMENTS_PAGES_BASE_URL is hardcoded; restore runtime configurability by reading the base URL from an environment/config value with a safe default (e.g., process.env.ELEMENTS_PAGES_BASE_URL || 'https://nvidia.github.io/elements') and use that variable when building startersData entries; update any references to ELEMENTS_PAGES_BASE_URL and ensure startersData (and any exported types) still export the same shape so consumers of startersData/Starter see no breaking change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@projects/internals/tools/src/project/starters.ts`:
- Around line 20-107: The constant ELEMENTS_PAGES_BASE_URL is hardcoded; restore
runtime configurability by reading the base URL from an environment/config value
with a safe default (e.g., process.env.ELEMENTS_PAGES_BASE_URL ||
'https://nvidia.github.io/elements') and use that variable when building
startersData entries; update any references to ELEMENTS_PAGES_BASE_URL and
ensure startersData (and any exported types) still export the same shape so
consumers of startersData/Starter see no breaking change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0a3ff961-926b-42ba-bd16-e536c4cdad07
📒 Files selected for processing (3)
projects/cli/package.jsonprojects/internals/tools/src/context/migration.mdprojects/internals/tools/src/project/starters.ts
💤 Files with no reviewable changes (1)
- projects/internals/tools/src/context/migration.md
1facd89 to
4354973
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
projects/internals/tools/src/project/starters.ts (1)
20-20:⚠️ Potential issue | 🟠 MajorRestore runtime configurability for the starter base URL.
Hardcoding
ELEMENTS_PAGES_BASE_URLremoves support for staging/mirror/air-gapped environments and makes all generated starter download URLs environment-inflexible. Keep the current URL as a default, but allow env override.Proposed fix
-import { cwd } from 'node:process'; +import { cwd, env } from 'node:process'; @@ -const ELEMENTS_PAGES_BASE_URL = 'https://nvidia.github.io/elements'; +const DEFAULT_ELEMENTS_PAGES_BASE_URL = 'https://nvidia.github.io/elements'; +const ELEMENTS_PAGES_BASE_URL = + env.ELEMENTS_PAGES_BASE_URL?.trim().replace(/\/+$/, '') || DEFAULT_ELEMENTS_PAGES_BASE_URL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/internals/tools/src/project/starters.ts` at line 20, The constant ELEMENTS_PAGES_BASE_URL is hardcoded; change it to read from an environment override with the current URL as the default so starter download URLs remain configurable at runtime. Replace the literal with a value derived from process.env.ELEMENTS_PAGES_BASE_URL (or your project's config/ENV accessor) falling back to 'https://nvidia.github.io/elements', and ensure any code that imports/uses ELEMENTS_PAGES_BASE_URL continues to get the resolved string.
🤖 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-global-attribute-value.test.ts`:
- Around line 8-11: The file imports the same default export twice
(noDeprecatedAttributeValue and noDeprecatedGlobalAttributeValue) which is
redundant; keep a single import (e.g., import noDeprecatedGlobalAttributeValue,
{ DEPRECATED_ATTRIBUTE_VALUES } from
'./no-deprecated-global-attribute-value.js') and update the rule assignment
(const rule = noDeprecatedGlobalAttributeValue as unknown as JSRuleDefinition)
plus any references later in the test (the assertions around
DEPRECATED_ATTRIBUTE_VALUES and usages between lines ~33-38) to use that single
symbol consistently; remove the duplicate import and adjust tests to reference
the preserved name.
- Around line 94-117: Add a regression test to the existing "multi-token
replacement" tester.run case that verifies expansion+dedup behavior: create an
invalid case where a deprecated token that expands into multiple tokens (e.g.,
'eyebrow' -> 'label sm' as defined by rewriteValue) overlaps with an existing
token in the attribute value (e.g., input value like "eyebrow label"), and
assert the autofix output deduplicates tokens (expected output "label sm") and
the error/suggestion use the same messageId
'unexpected-deprecated-global-attribute-value' and suggestion messageId
'suggest-replace-deprecated-global-attribute-value' so the test exercises
rewriteValue's Array.from(new Set(...)) dedup logic referenced in
no-deprecated-global-attribute-value.ts.
In `@projects/lint/src/eslint/rules/no-deprecated-global-attribute-value.ts`:
- Around line 12-19: DEPRECATED_ATTRIBUTE_VALUES duplicates the deprecated-token
definitions already present in the internals; instead, create a single shared
export in the internals (e.g., export a consolidated map from
projects/lint/src/eslint/internals/attributes.ts) and have both
DEPRECATED_ATTRIBUTE_VALUES (used by no-deprecated-global-attribute-value.ts)
and the logic in no-unexpected-global-attribute-value import and derive their
distilled-value filters and autofix mappings from that single shared export;
update references in the rule file (DEPRECATED_ATTRIBUTE_VALUES) and the other
rule to consume the shared symbol so changes are centralized and the two rules
can no longer drift.
---
Duplicate comments:
In `@projects/internals/tools/src/project/starters.ts`:
- Line 20: The constant ELEMENTS_PAGES_BASE_URL is hardcoded; change it to read
from an environment override with the current URL as the default so starter
download URLs remain configurable at runtime. Replace the literal with a value
derived from process.env.ELEMENTS_PAGES_BASE_URL (or your project's config/ENV
accessor) falling back to 'https://nvidia.github.io/elements', and ensure any
code that imports/uses ELEMENTS_PAGES_BASE_URL continues to get the resolved
string.
🪄 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: 5500ccf9-f327-4123-bd73-dd0d61468113
⛔ Files ignored due to path filters (3)
projects/core/.visual/icon.dark.pngis excluded by!**/*.pngprojects/core/.visual/icon.pngis excluded by!**/*.pngprojects/core/src/icon/icons/stop.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
projects/cli/package.jsonprojects/core/src/grid/grid.examples.tsprojects/core/src/icon/icons.tsprojects/core/src/icon/server.tsprojects/core/src/progress-bar/progress-bar.examples.tsprojects/internals/patterns/src/heatmap.examples.tsprojects/internals/tools/src/context/migration.mdprojects/internals/tools/src/project/starters.tsprojects/lint/README.mdprojects/lint/src/eslint/configs/html.tsprojects/lint/src/eslint/internals/attributes.tsprojects/lint/src/eslint/rules/no-deprecated-global-attribute-value.test.tsprojects/lint/src/eslint/rules/no-deprecated-global-attribute-value.tsprojects/lint/src/eslint/rules/no-unexpected-global-attribute-value.test.tsprojects/lint/src/eslint/rules/no-unexpected-global-attribute-value.tsprojects/site/src/_11ty/layouts/docs.11ty.jsprojects/site/src/docs/internal/guidelines/component-creation.mdprojects/site/src/docs/labs/index.mdprojects/site/src/docs/lint/index.mdprojects/site/src/docs/testing/index.md
💤 Files with no reviewable changes (3)
- projects/site/src/docs/labs/index.md
- projects/internals/tools/src/context/migration.md
- projects/site/src/docs/testing/index.md
| it('should rewrite deprecated tokens within a multi-token value', () => { | ||
| tester.run('multi-token replacement', rule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: '<div nve-layout="grow column"></div>', | ||
| output: '<div nve-layout="full column"></div>', | ||
| errors: [ | ||
| { | ||
| messageId: 'unexpected-deprecated-global-attribute-value', | ||
| data: { attribute: 'nve-layout', value: 'grow column', alternative: 'full column' }, | ||
| suggestions: [ | ||
| { | ||
| messageId: 'suggest-replace-deprecated-global-attribute-value', | ||
| data: { value: 'grow column', alternative: 'full column' }, | ||
| output: '<div nve-layout="full column"></div>' | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add a regression case for expansion+dedup token rewrites.
Current multi-token coverage validates grow -> full, but it does not pin the intentional dedup behavior when a deprecated token expands (for example, eyebrow -> label sm) and overlaps existing tokens.
🧪 Suggested test addition
invalid: [
{
code: '<div nve-layout="grow column"></div>',
output: '<div nve-layout="full column"></div>',
@@
}
]
+ },
+ {
+ code: '<div nve-text="label eyebrow"></div>',
+ output: '<div nve-text="label sm"></div>',
+ errors: [
+ {
+ messageId: 'unexpected-deprecated-global-attribute-value',
+ data: { attribute: 'nve-text', value: 'label eyebrow', alternative: 'label sm' },
+ suggestions: [
+ {
+ messageId: 'suggest-replace-deprecated-global-attribute-value',
+ data: { value: 'label eyebrow', alternative: 'label sm' },
+ output: '<div nve-text="label sm"></div>'
+ }
+ ]
+ }
+ ]
}
]Based on learnings: In projects/lint/src/eslint/rules/no-deprecated-global-attribute-value.ts, rewriteValue intentionally uses Array.from(new Set(...)) to deduplicate autofix tokens for multi-token replacements.
🤖 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-global-attribute-value.test.ts`
around lines 94 - 117, Add a regression test to the existing "multi-token
replacement" tester.run case that verifies expansion+dedup behavior: create an
invalid case where a deprecated token that expands into multiple tokens (e.g.,
'eyebrow' -> 'label sm' as defined by rewriteValue) overlaps with an existing
token in the attribute value (e.g., input value like "eyebrow label"), and
assert the autofix output deduplicates tokens (expected output "label sm") and
the error/suggestion use the same messageId
'unexpected-deprecated-global-attribute-value' and suggestion messageId
'suggest-replace-deprecated-global-attribute-value' so the test exercises
rewriteValue's Array.from(new Set(...)) dedup logic referenced in
no-deprecated-global-attribute-value.ts.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
projects/internals/tools/src/project/starters.ts (1)
20-108:⚠️ Potential issue | 🟠 MajorRestore a configurable pages base URL.
Hardcoding
https://nvidia.github.io/elementsremoves the mirror/staging/air-gapped escape hatch and makes starter generation less portable. Please keep an env/build-time override with the current URL as the default.Proposed fix
-import { cwd } from 'node:process'; +import { cwd, env } from 'node:process'; @@ -const ELEMENTS_PAGES_BASE_URL = 'https://nvidia.github.io/elements'; +const DEFAULT_ELEMENTS_PAGES_BASE_URL = 'https://nvidia.github.io/elements'; +const ELEMENTS_PAGES_BASE_URL = + env.ELEMENTS_PAGES_BASE_URL ?? DEFAULT_ELEMENTS_PAGES_BASE_URL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/internals/tools/src/project/starters.ts` around lines 20 - 108, Replace the hardcoded ELEMENTS_PAGES_BASE_URL with a configurable value that falls back to the current URL; e.g., derive ELEMENTS_PAGES_BASE_URL from an environment/build-time variable (like process.env.ELEMENTS_PAGES_BASE_URL or a build-time define) defaulting to 'https://nvidia.github.io/elements', and use that exported constant in startersData (symbols: ELEMENTS_PAGES_BASE_URL, startersData) so callers can override the base URL for mirrors/staging/air-gapped environments while preserving the existing default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@projects/internals/tools/src/project/starters.ts`:
- Around line 20-108: Replace the hardcoded ELEMENTS_PAGES_BASE_URL with a
configurable value that falls back to the current URL; e.g., derive
ELEMENTS_PAGES_BASE_URL from an environment/build-time variable (like
process.env.ELEMENTS_PAGES_BASE_URL or a build-time define) defaulting to
'https://nvidia.github.io/elements', and use that exported constant in
startersData (symbols: ELEMENTS_PAGES_BASE_URL, startersData) so callers can
override the base URL for mirrors/staging/air-gapped environments while
preserving the existing default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 633526b6-db81-4b44-8c5e-d5db89f6f7a3
📒 Files selected for processing (4)
projects/cli/package.jsonprojects/internals/tools/src/context/index.tsprojects/internals/tools/src/context/migration.mdprojects/internals/tools/src/project/starters.ts
💤 Files with no reviewable changes (1)
- projects/internals/tools/src/context/migration.md
437e65d to
594c667
Compare
Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
Signed-off-by: Cory Rylan <crylan@nvidia.com>
594c667 to
4f94a6c
Compare
Signed-off-by: Cory Rylan <crylan@nvidia.com>
|
🎉 This issue has been resolved in version 0.0.10 🎉 |
|
🎉 This issue has been resolved in version 0.1.0 🎉 |
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
3 similar comments
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
|
🎉 This issue has been resolved in version 0.0.10 🎉 |
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
1 similar comment
|
🎉 This issue has been resolved in version 0.0.8 🎉 |
Summary by CodeRabbit
New Features
Tests
Style / Examples
Documentation
Chores / Refactor