feat(core): add nve-format-number component#60
Open
jguzanvidia wants to merge 1 commit intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds a new nve-format-number web component that formats localized numbers via Intl.NumberFormat (decimal, currency, percent, unit, notations), plus tests (unit, Axe, Lighthouse, SSR), Storybook examples, docs, packaging exports, and minor ESLint/site integration updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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: 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/core/src/format-number/format-number.test.axe.ts`:
- Around line 12-23: The test currently calls removeFixture(fixture) after the
assertion so failures leave the DOM mounted; wrap the teardown in a try/finally:
after creating the fixture with createFixture(...) and awaiting
elementIsStable(...)/runAxe(...), ensure removeFixture(fixture) is invoked
inside a finally block so it always runs. Update the 'should pass axe check'
test to declare the fixture before the try, run assertions in try, and call
removeFixture(fixture) in finally; reference createFixture, fixture,
elementIsStable, runAxe, removeFixture, and FormatNumber.metadata.tag when
locating the test.
In `@projects/core/src/format-number/format-number.test.ts`:
- Around line 251-267: The fallback tests need to assert that LogService.warn is
called when invalid input/options are used: add a spy (e.g.,
jest.spyOn(LogService, 'warn')) at the start of each test that sets
element.number = 'not-a-number' and element.setAttribute('format-style',
'banana'), then perform the existing elementIsStable waits and the current DOM
assertions, and finally assert the spy was called (and optionally calledWith a
matching message) and restore the spy (spy.mockRestore()) to avoid leaking state
between tests; refer to element.number, element.setAttribute('format-style',
'banana'), elementIsStable, and LogService.warn to locate where to add the spy
and assertions.
🪄 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: 467bc4a4-0f46-4705-9c8c-fd107083141a
📒 Files selected for processing (15)
projects/core/eslint.config.jsprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/format-number/define.tsprojects/core/src/format-number/format-number.cssprojects/core/src/format-number/format-number.examples.tsprojects/core/src/format-number/format-number.test.axe.tsprojects/core/src/format-number/format-number.test.lighthouse.tsprojects/core/src/format-number/format-number.test.ssr.tsprojects/core/src/format-number/format-number.test.tsprojects/core/src/format-number/format-number.tsprojects/core/src/format-number/index.tsprojects/internals/eslint/src/configs/lit.jsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/format-number.md
• string and number properties mapping to Intl.NumberFormat options • decimal, currency, percent, and unit format styles • standard, scientific, engineering, and compact notation modes • LogService warnings for invalid values and invalid Intl options • inherits --nve-sys-text-color for theme compatibility • examples, full test suite and docs, visual tests skipped Signed-off-by: Jake Guza <jguza@nvidia.com>
jguzanvidia
force-pushed
the
topic/format-number
branch
from
e95736f to
f55f8f8
Compare
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/core/src/format-number/format-number.test.ts`:
- Around line 18-20: The test assigns the result of
fixture.querySelector(FormatNumber.metadata.tag) (type Element | null) directly
to element: FormatNumber causing strict type errors; update all such assignments
in this test file to use a type assertion (e.g., cast the querySelector result
to FormatNumber) so element is correctly typed, and keep the existing
null-safety flow by asserting before awaiting elementIsStable(element) — locate
uses around createFixture, FormatNumber.metadata.tag, and elementIsStable and
change querySelector(...) to a `as FormatNumber` assertion consistently.
In `@projects/core/src/format-number/format-number.ts`:
- Around line 34-49: The component's static readonly metadata.version in class
FormatNumber is still "0.0.0" and should be updated to the current release
version; locate the metadata object on the FormatNumber class and replace
metadata.version with the correct release string (or wire it to the
authoritative release constant/package version used by your build) so generated
docs/manifests reflect the actual release.
- Around line 136-145: The getter get `#parsedNumber` logs the full raw input on
invalid numbers via LogService.warn which can leak user-provided content; change
the warning to a generic/sanitized message (e.g., "format-number: invalid
numeric value" or include a redacted token like "<redacted>") and remove the raw
variable from the log call in the `#parsedNumber` implementation; update the
LogService.warn invocation in this function (and any related tests) so no
unredacted user content is emitted.
🪄 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: 882aaea8-4e43-41cb-bfc3-e5297bc5950f
📒 Files selected for processing (15)
projects/core/eslint.config.jsprojects/core/package.jsonprojects/core/src/bundle.tsprojects/core/src/format-number/define.tsprojects/core/src/format-number/format-number.cssprojects/core/src/format-number/format-number.examples.tsprojects/core/src/format-number/format-number.test.axe.tsprojects/core/src/format-number/format-number.test.lighthouse.tsprojects/core/src/format-number/format-number.test.ssr.tsprojects/core/src/format-number/format-number.test.tsprojects/core/src/format-number/format-number.tsprojects/core/src/format-number/index.tsprojects/internals/eslint/src/configs/lit.jsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/format-number.md
Comment on lines
+18
to
+20
| fixture = await createFixture(html`<nve-format-number locale="en-US">1234567</nve-format-number>`); | ||
| element = fixture.querySelector(FormatNumber.metadata.tag); | ||
| await elementIsStable(element); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP --type=ts --type=tsx 'export (async )?function createFixture|declare function createFixture' projects
rg -nP --type=ts --type=tsx 'static readonly metadata\s*=' projects/core/src/format-number/format-number.tsRepository: NVIDIA/elements
Length of output: 87
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for createFixture function definition
rg -nP --type=ts 'export (async )?function createFixture|declare function createFixture' projects
# Search for FormatNumber.metadata definition
rg -nP --type=ts 'static readonly metadata\s*=' projects/core/src/format-number/format-number.tsRepository: NVIDIA/elements
Length of output: 223
🏁 Script executed:
# Also get the full test file context around lines 18-20
head -n 35 projects/core/src/format-number/format-number.test.ts | tail -n +1Repository: NVIDIA/elements
Length of output: 1449
🏁 Script executed:
# Get the full createFixture implementation to see if there's any type narrowing
rg -A 10 'export async function createFixture' projects/internals/testing/src/index.ts
# Check the FormatNumber metadata to see the tag value
rg -A 5 'static readonly metadata' projects/core/src/format-number/format-number.ts
# Check if there's a type guard or assertion in the test file
rg -B 5 -A 10 'element = fixture.querySelector' projects/core/src/format-number/format-number.test.tsRepository: NVIDIA/elements
Length of output: 2516
Add type assertion to querySelector call.
fixture.querySelector(FormatNumber.metadata.tag) returns Element | null, but is assigned directly to element: FormatNumber. This violates strict type checking. Use as FormatNumber assertion:
element = fixture.querySelector(FormatNumber.metadata.tag) as FormatNumber;This pattern repeats throughout the test file and should be fixed consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/src/format-number/format-number.test.ts` around lines 18 - 20,
The test assigns the result of fixture.querySelector(FormatNumber.metadata.tag)
(type Element | null) directly to element: FormatNumber causing strict type
errors; update all such assignments in this test file to use a type assertion
(e.g., cast the querySelector result to FormatNumber) so element is correctly
typed, and keep the existing null-safety flow by asserting before awaiting
elementIsStable(element) — locate uses around createFixture,
FormatNumber.metadata.tag, and elementIsStable and change querySelector(...) to
a `as FormatNumber` assertion consistently.
Comment on lines
+34
to
+49
| /** | ||
| * @element nve-format-number | ||
| * @description A localized number formatter for currencies, percentages, units, and compact notation, backed by Intl.NumberFormat. | ||
| * Provide a `currency` attribute when `formatStyle` is `currency`, and a `unit` attribute when `formatStyle` is `unit`. | ||
| * @since 0.0.0 | ||
| * @entrypoint \@nvidia-elements/core/format-number | ||
| * @slot - Numeric string to format (such as 1234567 or 1234.56). Serves as fallback before hydration. | ||
| */ | ||
| @typeSSR() | ||
| export class FormatNumber extends LitElement { | ||
| static styles = useStyles([styles]); | ||
|
|
||
| static readonly metadata = { | ||
| tag: 'nve-format-number', | ||
| version: '0.0.0' | ||
| }; |
There was a problem hiding this comment.
Keep the component metadata version aligned.
metadata.version is still 0.0.0, which will make generated docs/manifest output look stale for a new public component. Align it with the current release metadata before shipping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/src/format-number/format-number.ts` around lines 34 - 49, The
component's static readonly metadata.version in class FormatNumber is still
"0.0.0" and should be updated to the current release version; locate the
metadata object on the FormatNumber class and replace metadata.version with the
correct release string (or wire it to the authoritative release constant/package
version used by your build) so generated docs/manifests reflect the actual
release.
Comment on lines
+136
to
+145
| get #parsedNumber(): number | null { | ||
| const raw = this.#rawValue; | ||
| if (!raw) return null; | ||
|
|
||
| const numericValue = Number(raw); | ||
| if (Number.isFinite(numericValue)) return numericValue; | ||
|
|
||
| LogService.warn(`format-number: invalid numeric value "${raw}"`); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Don't log the raw invalid value.
LogService.warn currently includes the full raw string. If invalid slot content comes from users, that leaks it into logs/telemetry. Please switch to a sanitized or generic message and keep the raw value out of the warning.
Suggested fix
- LogService.warn(`format-number: invalid numeric value "${raw}"`);
+ LogService.warn('format-number: invalid numeric value');📝 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
| get #parsedNumber(): number | null { | |
| const raw = this.#rawValue; | |
| if (!raw) return null; | |
| const numericValue = Number(raw); | |
| if (Number.isFinite(numericValue)) return numericValue; | |
| LogService.warn(`format-number: invalid numeric value "${raw}"`); | |
| return null; | |
| } | |
| get `#parsedNumber`(): number | null { | |
| const raw = this.#rawValue; | |
| if (!raw) return null; | |
| const numericValue = Number(raw); | |
| if (Number.isFinite(numericValue)) return numericValue; | |
| LogService.warn('format-number: invalid numeric value'); | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/src/format-number/format-number.ts` around lines 136 - 145, The
getter get `#parsedNumber` logs the full raw input on invalid numbers via
LogService.warn which can leak user-provided content; change the warning to a
generic/sanitized message (e.g., "format-number: invalid numeric value" or
include a redacted token like "<redacted>") and remove the raw variable from the
log call in the `#parsedNumber` implementation; update the LogService.warn
invocation in this function (and any related tests) so no unredacted user
content is emitted.
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
1 participant
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.
• string and number properties mapping to Intl.NumberFormat options • decimal, currency, percent, and unit format styles • standard, scientific, engineering, and compact notation modes • LogService warnings for invalid values and invalid Intl options • inherits --nve-sys-text-color for theme compatibility • examples, full test suite and docs, visual tests skipped
Summary by CodeRabbit
New Features
Documentation
Tests
Chores