feat(core): format-bytes - #205
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:
📝 WalkthroughWalkthroughThe PR adds the ChangesFormat Bytes component
Repository validation and maintenance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant FormatBytes
participant IntlNumberFormat
Consumer->>FormatBytes: Set value, unit, locale, and precision
FormatBytes->>IntlNumberFormat: Format converted byte value
IntlNumberFormat-->>FormatBytes: Return localized number
FormatBytes-->>Consumer: Render formatted value and unit label
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/format-bytes/format-bytes.test.axe.ts`:
- Around line 13-26: Update the stability setup in the test around
FormatBytes.metadata.tag to await elementIsStable for all three nve-format-bytes
fixture elements, rather than only the first querySelector result, before
calling runAxe. Preserve the existing fixture cleanup and accessibility
assertion.
In `@projects/core/src/format-bytes/format-bytes.ts`:
- Around line 130-146: Make display sourcing consistent between `#resolveAutoUnit`
and `#formatLabel`. Prefer passing the display value explicitly by updating
`#formatLabel` to accept a display parameter and updating its call in
`#formattedBytes` to pass this.display, while using that parameter for label
selection.
- Around line 148-167: Update `#warnInvalidOption` to remove the rawValue
parameter and return type/value, then adjust its callers in
`#hasValidConfiguration` to pass only the option name and value. Remove rawValue
from `#hasValidConfiguration` as well, and update its call site to invoke it
without arguments while preserving the existing invalid-configuration return
behavior.
- Around line 142-146: Update the formatting flow around `#formattedBytes` and
`#formatLabel` so the value is rounded once using the same fraction-digit settings
as `#formatNumber`, then pass that rounded value to both helpers. Select singular
versus plural in `#formatLabel` based on the rounded value while preserving
short-label behavior.
🪄 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: b30e9a98-6f6c-4e84-ac6e-c5b9e9faad96
⛔ Files ignored due to path filters (2)
projects/core/.visual/format-bytes.dark.pngis excluded by!**/*.pngprojects/core/.visual/format-bytes.pngis excluded by!**/*.png
📒 Files selected for processing (15)
projects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/format-bytes/define.tsprojects/core/src/format-bytes/format-bytes.cssprojects/core/src/format-bytes/format-bytes.examples.tsprojects/core/src/format-bytes/format-bytes.test.axe.tsprojects/core/src/format-bytes/format-bytes.test.lighthouse.tsprojects/core/src/format-bytes/format-bytes.test.ssr.tsprojects/core/src/format-bytes/format-bytes.test.tsprojects/core/src/format-bytes/format-bytes.test.visual.tsprojects/core/src/format-bytes/format-bytes.tsprojects/core/src/format-bytes/index.tsprojects/core/src/index.test.lighthouse.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/format-bytes.md
b57257b to
351a0f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/index.test.lighthouse.ts`:
- Line 18: The FormatBytes component definition import is missing from the
js-modules list used in the benchmark, so the new component is not included in
the aggregate payload measurement. Add the
`@nvidia-elements/core/format-bytes/define.js` import to the module-import list in
the test setup. After adding the import, run the benchmark to measure the actual
payload. Update the 134.5 KB threshold in the expect assertion only if the
measured payload for index.js exceeds the current limit.
🪄 Autofix
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: 660ec1ed-b543-4228-96dd-5fab6d84b4ae
⛔ Files ignored due to path filters (2)
projects/core/.visual/format-bytes.dark.pngis excluded by!**/*.pngprojects/core/.visual/format-bytes.pngis excluded by!**/*.png
📒 Files selected for processing (15)
projects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/format-bytes/define.tsprojects/core/src/format-bytes/format-bytes.cssprojects/core/src/format-bytes/format-bytes.examples.tsprojects/core/src/format-bytes/format-bytes.test.axe.tsprojects/core/src/format-bytes/format-bytes.test.lighthouse.tsprojects/core/src/format-bytes/format-bytes.test.ssr.tsprojects/core/src/format-bytes/format-bytes.test.tsprojects/core/src/format-bytes/format-bytes.test.visual.tsprojects/core/src/format-bytes/format-bytes.tsprojects/core/src/format-bytes/index.tsprojects/core/src/index.test.lighthouse.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/format-bytes.md
| @property({ type: Number, attribute: 'maximum-fraction-digits' }) maximumFractionDigits?: number; | ||
|
|
||
| get #rawValue(): string { | ||
| if (this.value === undefined) return this.textContent?.trim() ?? ''; |
There was a problem hiding this comment.
|| this.value === null)
Lit will set value to null if the value attribute is removed. Without this check, the component skips this branch and renders "null" instead of the text fallback.
(The other formatter components handle this with a nullish fallback.)
351a0f2 to
c46c74d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (1)
projects/core/src/format-bytes/format-bytes.ts:111
- When
valueis provided via the attribute and is not a valid number (e.g.value="not-a-number"), Lit'sNumberconverter setsthis.valuetoNaN. The current#rawValuethen becomes'NaN', so the component both warns about and rendersNaNrather than preserving the original input string (unlike invalid slot content, and unlikeformat-numberwhich preserves invalid attribute text).
get #rawValue(): string {
return String(this.value ?? this.textContent?.trim() ?? '');
}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/internals/eslint/src/local/no-missing-bundle-registration.js`:
- Around line 153-159: In no-missing-bundle-registration.js, replace raw-text
lighthouse import matching with AST-based collection of only static
ImportDeclaration module specifiers, preserving extraction of the component name
from valid `${escapedPrefix}/<component>/define.js` imports. In
projects/internals/eslint/src/local/no-missing-bundle-registration.test.js lines
109-137, add coverage proving matching text in comments or string literals still
reports missing-lighthouse-registration.
🪄 Autofix
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: 371bf729-2dbd-48c2-8432-5470988dba19
⛔ Files ignored due to path filters (2)
projects/core/.visual/format-bytes.dark.pngis excluded by!**/*.pngprojects/core/.visual/format-bytes.pngis excluded by!**/*.png
📒 Files selected for processing (23)
.agents/skills/component-creation/SKILL.mdconfig/vale/styles/config/vocabularies/Elements/accept.txtprojects/core/eslint.config.jsprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/format-bytes/define.tsprojects/core/src/format-bytes/format-bytes.cssprojects/core/src/format-bytes/format-bytes.examples.tsprojects/core/src/format-bytes/format-bytes.test.axe.tsprojects/core/src/format-bytes/format-bytes.test.lighthouse.tsprojects/core/src/format-bytes/format-bytes.test.ssr.tsprojects/core/src/format-bytes/format-bytes.test.tsprojects/core/src/format-bytes/format-bytes.test.visual.tsprojects/core/src/format-bytes/format-bytes.tsprojects/core/src/format-bytes/index.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/utils/keynav.test.tsprojects/internals/eslint/src/configs/lit.jsprojects/internals/eslint/src/local/no-missing-bundle-registration.jsprojects/internals/eslint/src/local/no-missing-bundle-registration.test.jsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/format-bytes.mdprojects/site/src/docs/metrics/api-status.11ty.js
💤 Files with no reviewable changes (2)
- config/vale/styles/config/vocabularies/Elements/accept.txt
- projects/internals/eslint/src/configs/lit.js
c46c74d to
edf2cd6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 @.agents/skills/component-creation/SKILL.md:
- Line 39: Update the component-creation guidance to require only the define.js
import in projects/core/src/bundle.ts, preserving its import-only registration
role. Remove the instruction to add a matching public export there, and direct
public export changes to the component or package export surface instead.
In `@projects/internals/eslint/README.md`:
- Line 112: Update the no-missing-bundle-test entry in the ESLint rule catalog
to document that it requires the lighthouseTestFile option, naming the aggregate
Lighthouse test file configured for the project. Keep the existing rule behavior
description and follow the repository’s Markdown technical-writing style.
In `@projects/internals/eslint/src/configs/lit.js`:
- Around line 70-71: Update the litConfig consumers in projects/code and
projects/monaco to enable local/no-missing-bundle-registration, ensuring
src/bundle.ts is enforced in both projects while preserving the existing
no-missing-bundle-test configuration.
In `@projects/internals/eslint/src/local/no-missing-bundle-test.js`:
- Around line 68-86: Move the duplicated getPackageName helper and the shared
prefix-escaping/importPattern construction into src/local/utils.js, then import
and reuse them from both no-missing-bundle-test.js and
no-missing-bundle-registration.js. Preserve the existing helper behavior and
generated import patterns while removing the duplicate local implementations.
- Around line 145-147: Update the missing-file branch in the Program:exit
handler to report a diagnostic when lighthouseTestPath does not exist instead of
returning silently. Add and use a dedicated missing-lighthouse-test messageId,
while preserving the existing validation flow when the file is present.
- Around line 103-118: Update the schema for the rule options object alongside
importPrefix and lighthouseTestFile to declare lighthouseTestFile as required.
Keep additionalProperties and the existing property definitions unchanged so
ESLint rejects configurations that omit this option.
- Around line 50-66: Update getModuleScriptSources to use optional chaining when
reading the script node value, so module ScriptTag nodes with undefined value
are skipped without throwing while preserving the existing string check and
collection behavior.
In `@projects/internals/eslint/src/local/no-missing-bundle-test.test.js`:
- Around line 20-41: Make both createBundleFixture helpers deterministic by
writing a package.json with name `@nvidia-elements/core` into each mkdtemp
directory before resolving the fixture. Update
projects/internals/eslint/src/local/no-missing-bundle-test.test.js lines 20-41
and projects/internals/eslint/src/local/no-missing-bundle-registration.test.js
lines 60-82; no other changes are needed.
In `@projects/site/src/docs/metrics/api-status.11ty.js`:
- Line 25: Add a regression assertion in the api-status test cases verifying
that an element named nve-json-viewer is retained by the filter while an element
whose name contains internal remains excluded. Keep the test focused on the
filtering behavior introduced in the api-status implementation.
🪄 Autofix
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: 8a2ab864-c427-42e1-be87-a33ecfc2550f
⛔ Files ignored due to path filters (2)
projects/core/.visual/format-bytes.dark.pngis excluded by!**/*.pngprojects/core/.visual/format-bytes.pngis excluded by!**/*.png
📒 Files selected for processing (25)
.agents/skills/component-creation/SKILL.mdconfig/vale/styles/config/vocabularies/Elements/accept.txtprojects/core/eslint.config.jsprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/format-bytes/define.tsprojects/core/src/format-bytes/format-bytes.cssprojects/core/src/format-bytes/format-bytes.examples.tsprojects/core/src/format-bytes/format-bytes.test.axe.tsprojects/core/src/format-bytes/format-bytes.test.lighthouse.tsprojects/core/src/format-bytes/format-bytes.test.ssr.tsprojects/core/src/format-bytes/format-bytes.test.tsprojects/core/src/format-bytes/format-bytes.test.visual.tsprojects/core/src/format-bytes/format-bytes.tsprojects/core/src/format-bytes/index.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/utils/keynav.test.tsprojects/internals/eslint/README.mdprojects/internals/eslint/src/configs/lit.jsprojects/internals/eslint/src/local/no-missing-bundle-registration.test.jsprojects/internals/eslint/src/local/no-missing-bundle-test.jsprojects/internals/eslint/src/local/no-missing-bundle-test.test.jsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/format-bytes.mdprojects/site/src/docs/metrics/api-status.11ty.js
💤 Files with no reviewable changes (1)
- config/vale/styles/config/vocabularies/Elements/accept.txt
| 'no-missing-bundle-registration': noMissingBundleRegistration, | ||
| 'no-missing-bundle-test': noMissingBundleTest, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every bundle.ts and check which eslint configs enable the bundle rules.
fd -t f 'bundle.ts' projects
fd -t f 'eslint.config.js' projects --exec rg -n 'no-missing-bundle-(registration|test)' {} \; -x echo "-- {}"
rg -n 'no-missing-bundle' projects --glob 'eslint.config.js' -C2Repository: NVIDIA/elements
Length of output: 871
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- lint configuration files ---'
fd -t f 'eslint.config.js' projects -0 | xargs -0 -r -n1 sh -c '
echo "--- $0"
cat -n "$0"
'
printf '%s\n' '--- litConfig usages ---'
rg -n -C3 'litConfig|configs/lit|eslint.*lit' projects --glob 'eslint.config.js' --glob 'package.json'
printf '%s\n' '--- bundle rule definitions and references ---'
rg -n -C4 'no-missing-bundle-(registration|test)' projects/internals projects --glob '*.{js,ts,json}' \
--glob '!**/node_modules/**'Repository: NVIDIA/elements
Length of output: 45372
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
root = Path("projects")
configs = sorted(root.rglob("eslint.config.js"))
bundles = sorted(
p for p in root.rglob("bundle.ts")
if "node_modules" not in p.parts
)
lit_consumers = []
severity_configs = []
for config in configs:
text = config.read_text()
if re.search(r"\blitConfig\b", text):
lit_consumers.append(config)
if re.search(r"['\"](?:local/)?no-missing-bundle-registration['\"]\s*:", text):
severity_configs.append(config)
print("bundle.ts files:")
for path in bundles:
print(f" {path}")
print("\nlitConfig consumers:")
for path in lit_consumers:
print(f" {path}")
print("\nconfigs with no-missing-bundle-registration severity:")
for path in severity_configs:
text = path.read_text()
for match in re.finditer(
r"['\"](?:local/)?no-missing-bundle-registration['\"]\s*:\s*([^,\n]+)",
text,
):
print(f" {path}: {match.group(1).strip()}")
print("\npackage/result matrix:")
for bundle in bundles:
package_dir = bundle.parent.parent
config = package_dir / "eslint.config.js"
consumes_lit = config.exists() and bool(re.search(r"\blitConfig\b", config.read_text()))
has_severity = config.exists() and bool(
re.search(r"['\"](?:local/)?no-missing-bundle-registration['\"]\s*:", config.read_text())
)
print(
f" {bundle}: config={config if config.exists() else 'none'}, "
f"litConfig={consumes_lit}, local severity={has_severity}"
)
PYRepository: NVIDIA/elements
Length of output: 984
Restore enforcement for src/bundle.ts in every litConfig consumer.
projects/code and projects/monaco consume litConfig and contain src/bundle.ts, but neither enables local/no-missing-bundle-registration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/internals/eslint/src/configs/lit.js` around lines 70 - 71, Update
the litConfig consumers in projects/code and projects/monaco to enable
local/no-missing-bundle-registration, ensuring src/bundle.ts is enforced in both
projects while preserving the existing no-missing-bundle-test configuration.
| ); | ||
| const elements = siteData.elements | ||
| .filter(element => !element.name.includes('internal') && !element.name.includes('json-viewer')) | ||
| .filter(element => !element.name.includes('internal')) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a regression test for json-viewer retention.
The changed filter now includes json-viewer, but the supplied projects/site/src/docs/metrics/api-status.test.ts cases do not verify this behavior. Add an assertion that nve-json-viewer remains included while elements containing internal remain excluded.
🤖 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/site/src/docs/metrics/api-status.11ty.js` at line 25, Add a
regression assertion in the api-status test cases verifying that an element
named nve-json-viewer is retained by the filter while an element whose name
contains internal remains excluded. Keep the test focused on the filtering
behavior introduced in the api-status implementation.
edf2cd6 to
ee3436a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/index.test.lighthouse.ts`:
- Line 102: Update the assertion in the buildPage payload test to validate the
intended JavaScript payload: either assert the expected request key before
checking its size, or switch to report.payload.javascript.kb if the 86.3 KB
limit applies across all code-split modules.
🪄 Autofix
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: 80082bb2-f9ec-4542-9d3b-46233b161891
⛔ Files ignored due to path filters (2)
projects/core/.visual/format-bytes.dark.pngis excluded by!**/*.pngprojects/core/.visual/format-bytes.pngis excluded by!**/*.png
📒 Files selected for processing (27)
.agents/skills/component-creation/SKILL.mdconfig/vale/styles/config/vocabularies/Elements/accept.txtknip.config.jsmise.tomlprojects/core/eslint.config.jsprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/format-bytes/define.tsprojects/core/src/format-bytes/format-bytes.cssprojects/core/src/format-bytes/format-bytes.examples.tsprojects/core/src/format-bytes/format-bytes.test.axe.tsprojects/core/src/format-bytes/format-bytes.test.lighthouse.tsprojects/core/src/format-bytes/format-bytes.test.ssr.tsprojects/core/src/format-bytes/format-bytes.test.tsprojects/core/src/format-bytes/format-bytes.test.visual.tsprojects/core/src/format-bytes/format-bytes.tsprojects/core/src/format-bytes/index.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/utils/keynav.test.tsprojects/internals/eslint/README.mdprojects/internals/eslint/src/configs/lit.jsprojects/internals/eslint/src/local/no-missing-bundle-registration.test.jsprojects/internals/eslint/src/local/no-missing-bundle-test.jsprojects/internals/eslint/src/local/no-missing-bundle-test.test.jsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/format-bytes.mdprojects/site/src/docs/metrics/api-status.11ty.js
💤 Files with no reviewable changes (2)
- knip.config.js
- config/vale/styles/config/vocabularies/Elements/accept.txt
ee3436a to
e7f5d5c
Compare
- Introduced `nve-format-bytes` component to convert byte counts into human-readable decimal or binary units Signed-off-by: Cory Rylan <crylan@nvidia.com>
- Updated the component creation guide to include new steps for updating shared entry points and measuring payloads. - Modified ESLint rules to enforce bundle registration and added support for Lighthouse benchmark integration. - Expanded Lighthouse test imports to include additional components and adjusted payload size expectations. - Removed deprecated component from vocabulary list and updated related documentation. Signed-off-by: Cory Rylan <crylan@nvidia.com>
e7f5d5c to
36a8274
Compare
nve-format-bytescomponent to convert byte counts into human-readable decimal or binary unitsSummary by CodeRabbit
New Features
Format Bytescomponent for localized decimal or binary byte formatting.Documentation
Tests