feat(core): format-bytes - #205
Open
coryrylan wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
scope(core)
scope(docs)
dependencies
📝 WalkthroughWalkthroughThe PR adds the ChangesFormat Bytes component
Repository validation and maintenance
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant FormatBytes
participant IntlNumberFormat
Consumer->>FormatBytes: Set value and formatting properties
FormatBytes->>IntlNumberFormat: Format value with locale and precision
IntlNumberFormat-->>FormatBytes: Return localized number
FormatBytes-->>Consumer: Render formatted value and unit label
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
coryrylan
force-pushed
the
topic-format-bytes
branch
from
August 4, 2026 14:31
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() ?? ''; |
Collaborator
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.)
- 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>
coryrylan
force-pushed
the
topic-format-bytes
branch
from
August 5, 2026 22:46
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
Comment on lines
+153
to
+159
| const lighthouseSource = readFileSync(lighthouseTestPath, 'utf8'); | ||
| const lighthouseImportPattern = new RegExp(`${escapedPrefix}/([^/]+)/define\\.js`, 'g'); | ||
| const lighthouseComponents = new Set(); | ||
|
|
||
| for (const match of lighthouseSource.matchAll(lighthouseImportPattern)) { | ||
| if (match[1]) lighthouseComponents.add(match[1]); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse Lighthouse imports as syntax, not raw text.
The regular expression accepts matching text in comments and string literals. A comment containing @nvidia-elements/core/card/define.js can add card to lighthouseComponents, so the bundle registration passes without a direct benchmark import.
projects/internals/eslint/src/local/no-missing-bundle-registration.js#L153-L159: collect only staticImportDeclarationmodule specifiers.projects/internals/eslint/src/local/no-missing-bundle-registration.test.js#L109-L137: add a case where matching comment or string text still reportsmissing-lighthouse-registration.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 153-153: Detects non-literal values in regular expressions
Context: new RegExp(${escapedPrefix}/([^/]+)/define\\.js, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity (ReDoS via non-literal RegExp).
(detect-non-literal-regexp)
📍 Affects 2 files
projects/internals/eslint/src/local/no-missing-bundle-registration.js#L153-L159(this comment)projects/internals/eslint/src/local/no-missing-bundle-registration.test.js#L109-L137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/internals/eslint/src/local/no-missing-bundle-registration.js` around
lines 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.
johnyanarella
self-requested a review
August 5, 2026 23:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
nve-format-bytescomponent to convert byte counts into human-readable decimal or binary unitsSummary by CodeRabbit
New Features
Format Byteselement for displaying byte values in decimal or binary units.Documentation
Tests