feat(core): progress gauge#154
Conversation
📝 WalkthroughWalkthroughAdds the ChangesProgress Gauge feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/core/src/bundle.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. projects/core/src/index.test.lighthouse.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. projects/core/src/progress-gauge/define.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
github-actions
Bot
added
scope(core)
scope(ci)
scope(docs)
dependencies
coryrylan
force-pushed
the
topic-progress-guage
branch
from
5675997 to
e315a0f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@config/vale/styles/config/vocabularies/Elements/accept.txt`:
- Around line 361-362: The Elements vocabulary entry currently includes generic
terms that should not be part of the shared docs vocabulary. Remove indicate and
utilization from the accept list in the Elements vocabulary file, and keep this
vocabulary limited to technical terms, component names, or abbreviations that
are specific to the project.
In `@projects/core/src/progress-gauge/progress-gauge.css`:
- Around line 4-21: The :host rule in progress-gauge.css fails
declaration-empty-line-before because there is no blank line before display;
update the :host block to add the required empty line separating the custom
property declarations from the standard declarations, keeping the fix within the
progress-gauge styles.
In `@projects/core/src/progress-gauge/progress-gauge.examples.ts`:
- Around line 114-123: The Dynamic example script creates an unmanaged interval
and uses a document-wide query, which can stack timers and keep mutating a
detached or wrong gauge on rerender. Update the inline module in
progress-gauge.examples.ts to scope lookups from the example’s own root
container instead of document, and keep a handle to the timer created in the
dynamic gauge example. Add teardown/cleanup for that setInterval when the
example is re-rendered or disposed so only one timer runs per instance.
In `@projects/core/src/progress-gauge/progress-gauge.test.axe.ts`:
- Around line 15-24: The axe test setup only waits for the first
nve-progress-gauge to stabilize, so the remaining gauges may still be rendering
when runAxe executes. Update the beforeEach in progress-gauge.test.axe.ts to
wait for every rendered ProgressGauge instance from the fixture, not just the
element returned by querySelector, so the DOM is fully settled before
accessibility checks run.
In `@projects/core/src/progress-gauge/progress-gauge.test.visual.ts`:
- Around line 19-24: The visual test template is always setting the nve-theme
attribute, even for the default baseline when theme is empty. Update template to
only call document.documentElement.setAttribute in the dark-theme case, and
leave the default document state untouched when theme === ''. Use the template
helper in progress-gauge.test.visual.ts as the place to guard the attribute
assignment.
In `@projects/core/src/progress-gauge/progress-gauge.ts`:
- Around line 22-26: The public version metadata for ProgressGauge still uses a
placeholder release value. Update the `@since` tag in the component docblock and
the ProgressGauge.metadata.version field so they reflect the real published
version instead of 0.0.0, and make the same change anywhere else in this diff
that duplicates the placeholder metadata.
- Around line 49-53: Normalize the progress gauge inputs before they are used in
geometry or ARIA: in progress-gauge.ts, clamp and sanitize `value` and `max` so
`NaN`, negative values, `max <= 0`, and `value > max` cannot flow into
rendering. Update `render()` and `updated()` to reuse the same normalized
numbers when computing the dash geometry and setting
`aria-valuenow`/`aria-valuemax`, using the `value` and `max` properties as the
source symbols to locate the fix.
🪄 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: a3a78298-8f17-4601-b576-e410d040447d
⛔ Files ignored due to path filters (2)
projects/core/.visual/progress-gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/progress-gauge.pngis excluded by!**/*.png
📒 Files selected for processing (16)
config/vale/styles/config/vocabularies/Elements/accept.txtprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/progress-gauge/define.tsprojects/core/src/progress-gauge/index.tsprojects/core/src/progress-gauge/progress-gauge.cssprojects/core/src/progress-gauge/progress-gauge.examples.tsprojects/core/src/progress-gauge/progress-gauge.test.axe.tsprojects/core/src/progress-gauge/progress-gauge.test.lighthouse.tsprojects/core/src/progress-gauge/progress-gauge.test.ssr.tsprojects/core/src/progress-gauge/progress-gauge.test.tsprojects/core/src/progress-gauge/progress-gauge.test.visual.tsprojects/core/src/progress-gauge/progress-gauge.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/progress-gauge.md
| indicate | ||
| utilization |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep the Elements vocabulary scoped to docs-specific terms.
indicate and utilization are generic words, so adding them here broadens the shared docs vocabulary without a clear technical need.
As per coding guidelines, add new technical terms, component names, or abbreviations to the Elements vocabulary.
♻️ Suggested change
-indicate
-utilization📝 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.
| indicate | |
| utilization |
🤖 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 `@config/vale/styles/config/vocabularies/Elements/accept.txt` around lines 361
- 362, The Elements vocabulary entry currently includes generic terms that
should not be part of the shared docs vocabulary. Remove indicate and
utilization from the accept list in the Elements vocabulary file, and keep this
vocabulary limited to technical terms, component names, or abbreviations that
are specific to the project.
Source: Coding guidelines
| function template(theme: '' | 'dark' = '') { | ||
| return /* html */ ` | ||
| <script type="module"> | ||
| import '@nvidia-elements/core/progress-gauge/define.js'; | ||
| document.documentElement.setAttribute('nve-theme', '${theme}'); | ||
| </script> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't set nve-theme on the default baseline.
When theme === '', this still adds an empty nve-theme attribute. That means the “default” snapshot is no longer exercising the actual default document state, and any selectors that depend on attribute presence can produce a false baseline.
Suggested fix
function template(theme: '' | 'dark' = '') {
return /* html */ `
<script type="module">
import '`@nvidia-elements/core/progress-gauge/define.js`';
- document.documentElement.setAttribute('nve-theme', '${theme}');
+ if ('${theme}') {
+ document.documentElement.setAttribute('nve-theme', '${theme}');
+ } else {
+ document.documentElement.removeAttribute('nve-theme');
+ }
</script>📝 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.
| function template(theme: '' | 'dark' = '') { | |
| return /* html */ ` | |
| <script type="module"> | |
| import '@nvidia-elements/core/progress-gauge/define.js'; | |
| document.documentElement.setAttribute('nve-theme', '${theme}'); | |
| </script> | |
| function template(theme: '' | 'dark' = '') { | |
| return /* html */ ` | |
| <script type="module"> | |
| import '`@nvidia-elements/core/progress-gauge/define.js`'; | |
| if ('${theme}') { | |
| document.documentElement.setAttribute('nve-theme', '${theme}'); | |
| } else { | |
| document.documentElement.removeAttribute('nve-theme'); | |
| } | |
| </script> |
🤖 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/core/src/progress-gauge/progress-gauge.test.visual.ts` around lines
19 - 24, The visual test template is always setting the nve-theme attribute,
even for the default baseline when theme is empty. Update template to only call
document.documentElement.setAttribute in the dark-theme case, and leave the
default document state untouched when theme === ''. Use the template helper in
progress-gauge.test.visual.ts as the place to guard the attribute assignment.
| /** | ||
| * @element nve-progress-gauge | ||
| * @description Use progress gauge to indicate system resource utilization. | ||
| * @since 0.0.0 | ||
| * @entrypoint \@nvidia-elements/core/progress-gauge |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Replace the 0.0.0 placeholders in the public version metadata.
@since and ProgressGauge.metadata.version still advertise 0.0.0, so generated docs and CLI/MCP consumers will expose the wrong release information for this component.
Suggested fix
- * `@since` 0.0.0
+ * `@since` 2.0.2
@@
static readonly metadata = {
tag: 'nve-progress-gauge',
- version: '0.0.0'
+ version: '2.0.2'
};Also applies to: 41-44
🤖 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/core/src/progress-gauge/progress-gauge.ts` around lines 22 - 26, The
public version metadata for ProgressGauge still uses a placeholder release
value. Update the `@since` tag in the component docblock and the
ProgressGauge.metadata.version field so they reflect the real published version
instead of 0.0.0, and make the same change anywhere else in this diff that
duplicates the placeholder metadata.
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-progress-guage
branch
from
e315a0f to
a9cd207
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/core/src/progress-gauge/progress-gauge.css`:
- Around line 96-99: The `::slotted(*)` rule in `progress-gauge.css` is
overriding every assigned child’s typography, including consumer-provided styles
like the muted secondary label in the progress gauge example. Remove the blanket
`color` and `font-size` application from the slot styling and let slotted
content inherit defaults instead, keeping any necessary styling scoped to the
gauge’s internal elements rather than all top-level slotted nodes.
In `@projects/core/src/progress-gauge/progress-gauge.test.ts`:
- Around line 18-19: The test setup in progress-gauge.test.ts assigns the result
of ProgressGauge.metadata.tag from querySelector() directly to element, which
can be null under strict typing. Update the setup to guard or assert the
querySelector() result before calling elementIsStable() and before any
_internals access, using the existing element variable and the
ProgressGauge.metadata.tag lookup so a missing custom element fails clearly
instead of crashing later.
In `@projects/core/src/progress-gauge/progress-gauge.ts`:
- Around line 58-59: The `container` property in `ProgressGauge` can receive
arbitrary HTML strings, so `geometry` lookup in `render()` must not assume only
valid values; guard the value before indexing `GAUGE_GEOMETRY` and fall back to
the default geometry unless `container` is exactly `'half'`. Update the
`ProgressGauge` type handling and the `geometry` selection logic together so
`render()` never sees `undefined` for invalid `container` values.
🪄 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: ad27c55d-a5d0-47d8-857a-91c33b25067d
⛔ Files ignored due to path filters (2)
projects/core/.visual/progress-gauge.dark.pngis excluded by!**/*.pngprojects/core/.visual/progress-gauge.pngis excluded by!**/*.png
📒 Files selected for processing (15)
projects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/progress-gauge/define.tsprojects/core/src/progress-gauge/index.tsprojects/core/src/progress-gauge/progress-gauge.cssprojects/core/src/progress-gauge/progress-gauge.examples.tsprojects/core/src/progress-gauge/progress-gauge.test.axe.tsprojects/core/src/progress-gauge/progress-gauge.test.lighthouse.tsprojects/core/src/progress-gauge/progress-gauge.test.ssr.tsprojects/core/src/progress-gauge/progress-gauge.test.tsprojects/core/src/progress-gauge/progress-gauge.test.visual.tsprojects/core/src/progress-gauge/progress-gauge.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/progress-gauge.md
| ::slotted(*) { | ||
| color: var(--color); | ||
| font-size: var(--font-size); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not override all consumer slotted typography.
::slotted(*) forces the same color and font-size onto every top-level child. That breaks multi-line content like projects/core/src/progress-gauge/progress-gauge.examples.ts Lines 110-113, where the secondary GPU label is explicitly marked nve-text="body sm muted" but will still render with the primary gauge text styling. Let the slot provide defaults via inheritance instead of restyling every assigned node.
Proposed fix
-::slotted(*) {
- color: var(--color);
- font-size: var(--font-size);
-}📝 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.
| ::slotted(*) { | |
| color: var(--color); | |
| font-size: var(--font-size); | |
| } |
🤖 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/core/src/progress-gauge/progress-gauge.css` around lines 96 - 99,
The `::slotted(*)` rule in `progress-gauge.css` is overriding every assigned
child’s typography, including consumer-provided styles like the muted secondary
label in the progress gauge example. Remove the blanket `color` and `font-size`
application from the slot styling and let slotted content inherit defaults
instead, keeping any necessary styling scoped to the gauge’s internal elements
rather than all top-level slotted nodes.
| element = fixture.querySelector(ProgressGauge.metadata.tag); | ||
| await elementIsStable(element); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "TypeScript config(s):"
fd -i 'tsconfig*.json' . -x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {}
echo
echo "Unsafe ProgressGauge fixture lookups:"
rg -n -C2 'querySelector\(ProgressGauge\.metadata\.tag\)' projects/core/src/progress-gauge/progress-gauge.test.tsRepository: NVIDIA/elements
Length of output: 20626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant test file:"
sed -n '1,260p' projects/core/src/progress-gauge/progress-gauge.test.ts
echo
echo "elementIsStable definition:"
rg -n -C3 'function elementIsStable|const elementIsStable|export .*elementIsStable' projects/internals/testing projects/core/src -g '*.ts'
echo
echo "ProgressGauge class and metadata:"
rg -n -C3 'class ProgressGauge|metadata\.tag' projects/core/src/progress-gauge -g '*.ts'Repository: NVIDIA/elements
Length of output: 18735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "TypeScript DOM querySelector typings in installed libs, if available:"
node - <<'JS'
const fs = require('fs');
const path = require('path');
const candidates = [
'node_modules/typescript/lib/lib.dom.d.ts',
'/usr/lib/node_modules/typescript/lib/lib.dom.d.ts',
'/usr/local/lib/node_modules/typescript/lib/lib.dom.d.ts',
];
for (const p of candidates) {
if (fs.existsSync(p)) {
const text = fs.readFileSync(p, 'utf8');
const idx = text.indexOf('querySelector<K extends keyof HTMLElementTagNameMap>');
if (idx !== -1) {
console.log(`FOUND ${p}`);
console.log(text.slice(idx, idx + 700));
process.exit(0);
}
}
}
console.log('lib.dom.d.ts not found in candidate locations');
JSRepository: NVIDIA/elements
Length of output: 986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "createFixture definition:"
sed -n '1,220p' projects/internals/testing/src/index.ts
echo
echo "progress-gauge test fixture patterns in nearby tests:"
rg -n -C2 'fixture\.querySelector\(ProgressGauge\.metadata\.tag\)|querySelector\(ProgressGauge\.metadata\.tag\)' projects/core/src/progress-gauge -g '*.test.ts'Repository: NVIDIA/elements
Length of output: 6212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' projects/core/src/progress-gauge/progress-gauge.tsRepository: NVIDIA/elements
Length of output: 4065
Guard the querySelector() result before assigning element. querySelector() can return null, so these direct assignments are unsafe under strict TS and can turn a missing custom element into an opaque setup crash. Add an assertion/helper before elementIsStable() and the _internals reads.
🤖 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/core/src/progress-gauge/progress-gauge.test.ts` around lines 18 -
19, The test setup in progress-gauge.test.ts assigns the result of
ProgressGauge.metadata.tag from querySelector() directly to element, which can
be null under strict typing. Update the setup to guard or assert the
querySelector() result before calling elementIsStable() and before any
_internals access, using the existing element variable and the
ProgressGauge.metadata.tag lookup so a missing custom element fails clearly
instead of crashing later.
Source: Coding guidelines
| /** Determines the gauge container shape. Set `half` for a compact semi-circular arc. */ | ||
| @property({ type: String, reflect: true }) container?: 'half'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard invalid container values before indexing GAUGE_GEOMETRY.
HTML attributes are runtime strings, so <nve-progress-gauge container="foo"> makes geometry undefined and render() throws on Line 83. Fall back explicitly unless the value is exactly 'half'. As per coding guidelines, "When working with TypeScript code, follow /projects/site/src/docs/internal/guidelines/typescript.md for type safety, type guards, discriminated unions, and exhaustive checking."
Suggested fix
- const geometry = GAUGE_GEOMETRY[this.container ?? 'default'];
+ const geometry =
+ this.container === 'half' ? GAUGE_GEOMETRY.half : GAUGE_GEOMETRY.default;Also applies to: 76-79
🤖 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/core/src/progress-gauge/progress-gauge.ts` around lines 58 - 59, The
`container` property in `ProgressGauge` can receive arbitrary HTML strings, so
`geometry` lookup in `render()` must not assume only valid values; guard the
value before indexing `GAUGE_GEOMETRY` and fall back to the default geometry
unless `container` is exactly `'half'`. Update the `ProgressGauge` type handling
and the `geometry` selection logic together so `render()` never sees `undefined`
for invalid `container` values.
Source: Coding guidelines
1 participant
Summary by CodeRabbit
New Features
Bug Fixes