feat(core): format-bytes - #205
Open
coryrylan wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
scope(core)
scope(docs)
dependencies
📝 WalkthroughWalkthroughThe PR adds the ChangesFormat Bytes component
Estimated code review effort: 3 (Moderate) | ~25 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
Comment on lines
+13
to
+26
| const fixture = await createFixture(html` | ||
| <nve-format-bytes locale="en-US">1048576</nve-format-bytes> | ||
| <nve-format-bytes locale="en-US" display="binary" unit-display="long">1048576</nve-format-bytes> | ||
| <nve-format-bytes locale="de-DE" unit="kb">1048576</nve-format-bytes> | ||
| `); | ||
|
|
||
| try { | ||
| await elementIsStable(fixture.querySelector(FormatBytes.metadata.tag)); | ||
| const results = await runAxe([FormatBytes.metadata.tag]); | ||
| expect(results.violations.length).toBe(0); | ||
| } finally { | ||
| removeFixture(fixture); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Await stability for all three fixture elements, not just the first.
The fixture creates three nve-format-bytes elements. Line 20 only awaits elementIsStable on fixture.querySelector(FormatBytes.metadata.tag), which resolves to the first matching element only. The axe scan on line 21 covers all three elements, but the second and third elements' render cycles are not confirmed stable. This can produce a flaky or false-negative accessibility result if axe scans an element mid-update.
🐛 Proposed fix to await stability for every element
try {
- await elementIsStable(fixture.querySelector(FormatBytes.metadata.tag));
+ await Promise.all(
+ [...fixture.querySelectorAll(FormatBytes.metadata.tag)].map((el) => elementIsStable(el))
+ );
const results = await runAxe([FormatBytes.metadata.tag]);📝 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.
Suggested change
| const fixture = await createFixture(html` | |
| <nve-format-bytes locale="en-US">1048576</nve-format-bytes> | |
| <nve-format-bytes locale="en-US" display="binary" unit-display="long">1048576</nve-format-bytes> | |
| <nve-format-bytes locale="de-DE" unit="kb">1048576</nve-format-bytes> | |
| `); | |
| try { | |
| await elementIsStable(fixture.querySelector(FormatBytes.metadata.tag)); | |
| const results = await runAxe([FormatBytes.metadata.tag]); | |
| expect(results.violations.length).toBe(0); | |
| } finally { | |
| removeFixture(fixture); | |
| } | |
| }); | |
| const fixture = await createFixture(html` | |
| <nve-format-bytes locale="en-US">1048576</nve-format-bytes> | |
| <nve-format-bytes locale="en-US" display="binary" unit-display="long">1048576</nve-format-bytes> | |
| <nve-format-bytes locale="de-DE" unit="kb">1048576</nve-format-bytes> | |
| `); | |
| try { | |
| await Promise.all( | |
| [...fixture.querySelectorAll(FormatBytes.metadata.tag)].map((el) => elementIsStable(el)) | |
| ); | |
| const results = await runAxe([FormatBytes.metadata.tag]); | |
| expect(results.violations.length).toBe(0); | |
| } finally { | |
| removeFixture(fixture); | |
| } | |
| }); |
🤖 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/format-bytes/format-bytes.test.axe.ts` around lines 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.
- Introduced `nve-format-bytes` component to convert byte counts into human-readable decimal or binary units Signed-off-by: Cory Rylan <crylan@nvidia.com>
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
| expect(report.scores.accessibility).toBe(100); | ||
| expect(report.scores.bestPractices).toBe(100); | ||
| expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(133.6); | ||
| expect(report.payload.javascript.requests['index.js'].kb).toBeLessThan(134.5); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Include FormatBytes in the module-import bundle benchmark.
@nvidia-elements/core/format-bytes/define.js is absent from the js-modules import list. The benchmark does not measure the new component in the aggregate direct-import payload. Add the definition import. Adjust the 82 KB limit only if the measured payload requires it.
Proposed change
import '`@nvidia-elements/core/file/define.js`';
+ import '`@nvidia-elements/core/format-bytes/define.js`';
import '`@nvidia-elements/core/forms/define.js`';🤖 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/index.test.lighthouse.ts` at 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.
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