feat: posture rounding clarity - BED-7595#2474
feat: posture rounding clarity - BED-7595#2474dcairnsspecterops wants to merge 9 commits intomainfrom
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:
📝 WalkthroughWalkthroughAdds a THRESHOLDS type and threshold-driven behavior to number formatting: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts (1)
17-25: Export the new threshold contract.
abbreviatedNumbernow exposes this shape in its public signature, butTHRESHOLDSis private to the module. That makes downstream TS callers duplicate the object shape instead of importing the canonical type.♻️ Proposed refactor
-type THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number }; +export type NumberFormattingThresholds = { + abbreviationThreshold: number; + decimalDigitThreshold: number; +}; export const abbreviatedNumber = ( num: number, fractionDigits: number = 1, - { abbreviationThreshold, decimalDigitThreshold }: THRESHOLDS = { + { abbreviationThreshold, decimalDigitThreshold }: NumberFormattingThresholds = { abbreviationThreshold: 0, decimalDigitThreshold: 0, } ) => {Based on learnings: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts` around lines 17 - 25, The file defines a private type alias THRESHOLDS used in the public function abbreviatedNumber; export that type so consumers can import the canonical shape instead of duplicating it. Change the module-level declaration to an exported type (e.g., export type THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number }) and then add an export from the package API surface (re-export THRESHOLDS from the package barrel/index) so downstream projects can import THRESHOLDS alongside abbreviatedNumber.packages/javascript/bh-shared-ui/src/utils/numberFormatting.test.ts (1)
53-101: Add assertions at the exact cutovers.These tests cover representative values, but the requirement is really about the boundaries. Please add explicit expectations for
99_999staying comma-separated and100_000switching to100Kso an off-by-one regression at the threshold gets caught quickly. You may also want the same treatment around the decimal cutover if that behavior is intended to stay stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.test.ts` around lines 53 - 101, Add explicit boundary assertions around the abbreviation threshold for the abbreviatedNumber function: add tests that abbreviatedNumber(99_999, <same precision>, THRESHOLDS) returns '99,999' and abbreviatedNumber(100_000, <same precision>, THRESHOLDS) returns '100K' (use the same THRESHOLDS constant and matching precision used elsewhere in this file). Also add equivalent cutoff assertions for the decimalDigitThreshold behavior (using decimalDigitThreshold from THRESHOLDS) to verify the exact decimal cutover remains stable (e.g., values just below and at the decimal threshold produce comma-separated vs. abbreviated/decimal outputs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.test.ts`:
- Around line 53-101: Add explicit boundary assertions around the abbreviation
threshold for the abbreviatedNumber function: add tests that
abbreviatedNumber(99_999, <same precision>, THRESHOLDS) returns '99,999' and
abbreviatedNumber(100_000, <same precision>, THRESHOLDS) returns '100K' (use the
same THRESHOLDS constant and matching precision used elsewhere in this file).
Also add equivalent cutoff assertions for the decimalDigitThreshold behavior
(using decimalDigitThreshold from THRESHOLDS) to verify the exact decimal
cutover remains stable (e.g., values just below and at the decimal threshold
produce comma-separated vs. abbreviated/decimal outputs).
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Around line 17-25: The file defines a private type alias THRESHOLDS used in
the public function abbreviatedNumber; export that type so consumers can import
the canonical shape instead of duplicating it. Change the module-level
declaration to an exported type (e.g., export type THRESHOLDS = {
abbreviationThreshold: number; decimalDigitThreshold: number }) and then add an
export from the package API surface (re-export THRESHOLDS from the package
barrel/index) so downstream projects can import THRESHOLDS alongside
abbreviatedNumber.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 60408aba-a221-49f8-bb50-928def881130
📒 Files selected for processing (2)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.test.tspackages/javascript/bh-shared-ui/src/utils/numberFormatting.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts (2)
40-45:⚠️ Potential issue | 🟡 MinorPre-existing:
abbreviationsarray lacks entry for quadrillions (Q).Numbers in the quadrillions range (10^15 to ~9×10^15) are safe integers but would produce
log1000 = 5, exceeding the array bounds (indices 0-4). This would result in output like"9.007undefined".Consider adding
'Q'to the abbreviations array to handle this magnitude, or adjusting the safe integer check threshold.🛡️ Suggested fix
- const abbreviations = ['', 'K', 'M', 'B', 'T']; + const abbreviations = ['', 'K', 'M', 'B', 'T', 'Q'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts` around lines 40 - 45, The abbreviations array used by the formatting logic (abbreviations) doesn't include an entry for quadrillions, so when log1000 evaluates to 5 (e.g., num ~10^15) the code (using log1000 and abbreviations) will read out of bounds; update the abbreviations array to include 'Q' (so entries become ['', 'K', 'M', 'B', 'T', 'Q']) or otherwise guard/limit log1000 before using it, and ensure the formattedNumber + abbreviations[log1000] concatenation always accesses a valid index (refer to abbreviations and log1000 in numberFormatting.ts).
40-45:⚠️ Potential issue | 🟡 MinorEdge case:
num = 0with default thresholds produces invalid output.When
numis0and no thresholds are provided (using defaults),Math.log10(0)returns-Infinity, causinglog1000to be-Infinity. This results inabbreviations[-Infinity]returningundefinedand the division producingNaN, yielding output like"NaNundefined".This is likely a pre-existing issue, but the new threshold-based early return at line 30 masks it when thresholds are set. Consider adding an explicit check for zero.
🛡️ Suggested fix
if (!Number.isSafeInteger(num)) return '>9Q'; + if (num === 0) return '0'; + if (abbreviationThreshold && num < abbreviationThreshold) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts` around lines 40 - 45, Add an explicit check for num === 0 before computing Math.log10 so you don't compute log1000 from -Infinity; in the number formatting utility (around the abbreviations, log1000 and formattedNumber code) return a properly formatted zero (e.g., (0).toFixed(fractionDigits) with the empty abbreviation) when num is 0 instead of proceeding to Math.log10/Math.pow which produces NaN/undefined.
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts (1)
17-17: Consider exporting theTHRESHOLDStype and using PascalCase naming.The type is used as a parameter in the public
abbreviatedNumberfunction. If consumers need to pass custom thresholds with proper type safety, they'll need access to this type. Additionally, TypeScript convention typically uses PascalCase for types (e.g.,Thresholds) rather than SCREAMING_CASE which is usually reserved for constants.♻️ Suggested change
-type THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number }; +export type Thresholds = { abbreviationThreshold: number; decimalDigitThreshold: number };Then update the function signature accordingly.
Based on learnings: "In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package's API surface so they can be consumed by parent projects."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts` at line 17, Rename the THRESHOLDS type to PascalCase (Thresholds), export it, and update the public abbreviatedNumber function signature to accept the new exported type (e.g., abbreviatedNumber(value: number, thresholds?: Thresholds)); ensure the type shape remains { abbreviationThreshold: number; decimalDigitThreshold: number } and add an export for Thresholds so consumers get proper type safety, then update the package's public API exports to re-export Thresholds (so parent projects can import it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Around line 40-45: The abbreviations array used by the formatting logic
(abbreviations) doesn't include an entry for quadrillions, so when log1000
evaluates to 5 (e.g., num ~10^15) the code (using log1000 and abbreviations)
will read out of bounds; update the abbreviations array to include 'Q' (so
entries become ['', 'K', 'M', 'B', 'T', 'Q']) or otherwise guard/limit log1000
before using it, and ensure the formattedNumber + abbreviations[log1000]
concatenation always accesses a valid index (refer to abbreviations and log1000
in numberFormatting.ts).
- Around line 40-45: Add an explicit check for num === 0 before computing
Math.log10 so you don't compute log1000 from -Infinity; in the number formatting
utility (around the abbreviations, log1000 and formattedNumber code) return a
properly formatted zero (e.g., (0).toFixed(fractionDigits) with the empty
abbreviation) when num is 0 instead of proceeding to Math.log10/Math.pow which
produces NaN/undefined.
---
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Line 17: Rename the THRESHOLDS type to PascalCase (Thresholds), export it, and
update the public abbreviatedNumber function signature to accept the new
exported type (e.g., abbreviatedNumber(value: number, thresholds?: Thresholds));
ensure the type shape remains { abbreviationThreshold: number;
decimalDigitThreshold: number } and add an export for Thresholds so consumers
get proper type safety, then update the package's public API exports to
re-export Thresholds (so parent projects can import it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 88a799d8-2ce0-493b-8d3d-6bf408c02bec
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts (1)
41-45:⚠️ Potential issue | 🟠 MajorHandle rollover when rounding crosses into the next suffix.
At the K/M boundary this can emit invalid values like
1000Kafter rounding. For example, withfractionDigits = 0,999500becomes1000K, which does not match thexxxK / y.yyyM / z.zzzBformat in the requirement.🐛 Proposed fix
- const log1000 = Math.floor(Math.log10(Math.abs(num)) / 3); // appropriate abbreviation index + let log1000 = Math.floor(Math.log10(Math.abs(num)) / 3); // appropriate abbreviation index // Otherwise, divide the number by the appropriate power of 1000 and add the abbreviation - const formattedNumber = (num / Math.pow(1000, log1000)).toFixed(fractionDigits); + let formattedNumber = (num / Math.pow(1000, log1000)).toFixed(fractionDigits); + + if (Math.abs(Number(formattedNumber)) >= 1000 && log1000 < abbreviations.length - 1) { + log1000 += 1; + formattedNumber = (num / Math.pow(1000, log1000)).toFixed(fractionDigits); + } + return formattedNumber + abbreviations[log1000];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts` around lines 41 - 45, The current logic can produce rollovers like "1000K" when rounding pushes the mantissa to 1000; after computing log1000 and formattedNumber, parse the numeric mantissa (e.g., parseFloat(formattedNumber)) and check if it is >= 1000 (or >= Math.pow(1000,1) relative to your unit). If it is, set the mantissa to mantissa / 1000 (or 1) and increment log1000 by 1, then recompute the formatted string and suffix using the updated log1000 and abbreviations; also guard against exceeding abbreviations.length and fall back to the highest suffix if needed. Apply this fix around the block that computes log1000, formattedNumber, and the final return so that values like 999500 with fractionDigits=0 become "1M" (or "1.0M" per fractionDigits) instead of "1000K".
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts (1)
17-25: Export the new thresholds type with the public utility.
THRESHOLDSis now part ofabbreviatedNumber’s public contract, but callers cannot reuse the named type from this module. Exporting it avoids downstream shape duplication.♻️ Suggested change
-type THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number }; +export type THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number };Based on learnings, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts` around lines 17 - 25, The THRESHOLDS type used by the exported function abbreviatedNumber is part of the function's public API but is not exported; export THRESHOLDS so callers can reuse the type instead of duplicating its shape. Update the module to export the type alias (THRESHOLDS) alongside abbreviatedNumber (e.g., export type THRESHOLDS = ... or add export to the existing declaration) and ensure any imports/exports in the package index re-export it as part of the public API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Around line 30-37: The threshold comparisons in numberFormatting.ts are using
the signed value (num < abbreviationThreshold and num < decimalDigitThreshold),
which misclassifies large negative numbers; change both checks to compare the
magnitude using Math.abs(num) (e.g., replace num < abbreviationThreshold and num
< decimalDigitThreshold with Math.abs(num) < abbreviationThreshold and
Math.abs(num) < decimalDigitThreshold) so functions like commaSeparatedNumber
and the fractionDigits logic apply based on absolute value of num.
---
Outside diff comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Around line 41-45: The current logic can produce rollovers like "1000K" when
rounding pushes the mantissa to 1000; after computing log1000 and
formattedNumber, parse the numeric mantissa (e.g., parseFloat(formattedNumber))
and check if it is >= 1000 (or >= Math.pow(1000,1) relative to your unit). If it
is, set the mantissa to mantissa / 1000 (or 1) and increment log1000 by 1, then
recompute the formatted string and suffix using the updated log1000 and
abbreviations; also guard against exceeding abbreviations.length and fall back
to the highest suffix if needed. Apply this fix around the block that computes
log1000, formattedNumber, and the final return so that values like 999500 with
fractionDigits=0 become "1M" (or "1.0M" per fractionDigits) instead of "1000K".
---
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Around line 17-25: The THRESHOLDS type used by the exported function
abbreviatedNumber is part of the function's public API but is not exported;
export THRESHOLDS so callers can reuse the type instead of duplicating its
shape. Update the module to export the type alias (THRESHOLDS) alongside
abbreviatedNumber (e.g., export type THRESHOLDS = ... or add export to the
existing declaration) and ensure any imports/exports in the package index
re-export it as part of the public API surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c2973c17-dc24-40b7-a2ae-76c1f328bd29
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts (1)
17-17: Type naming convention: prefer PascalCase.TypeScript convention uses PascalCase for type aliases (e.g.,
Thresholds). All-caps is typically reserved for constants.✨ Suggested rename
-type THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number }; +type Thresholds = { abbreviationThreshold: number; decimalDigitThreshold: number };Then update the parameter type on line 22 accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts` at line 17, Rename the type alias THRESHOLDS to PascalCase Thresholds and update all references to use the new name; specifically change the declaration "type THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number }" to "type Thresholds = {...}" and update the parameter type on the function that currently accepts THRESHOLDS (the function signature that mentions abbreviationThreshold/decimalDigitThreshold) to use Thresholds instead; ensure any imports/exports and tests referencing THRESHOLDS are updated to Thresholds as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Around line 30-34: The branch that returns commaSeparatedNumber(absNum) for
values below abbreviationThreshold loses the original sign for negative inputs;
update the logic in numberFormatting (the block using absNum,
abbreviationThreshold, and commaSeparatedNumber) to detect if num is negative
and prefix the formatted string with a '-' (or otherwise preserve the sign)
before returning, ensuring zero remains unchanged and positive numbers are
unchanged.
- Around line 45-46: The abbreviated formatter currently computes the magnitude
from absNum but drops the original sign; in the function that defines absNum,
log1000, formattedNumber and uses abbreviations[log1000], preserve the sign by
determining it from the original input (e.g., num or number) and prefixing '-'
when negative before returning. Concretely, keep the value calculation as
(absNum / Math.pow(1000, log1000)).toFixed(fractionDigits) but return
(originalValue < 0 ? '-' : '') + formattedValue + abbreviations[log1000],
ensuring zero and positive numbers remain unchanged.
- Around line 36-39: The current falsy check "if (decimalDigitThreshold &&
absNum < decimalDigitThreshold)" skips when decimalDigitThreshold is 0; change
the condition to explicitly test for presence (e.g., decimalDigitThreshold !==
undefined && decimalDigitThreshold !== null) so a valid 0 threshold is
respected, then keep the absNum < decimalDigitThreshold check and set
fractionDigits = 0; reference decimalDigitThreshold, absNum, and fractionDigits
in your fix.
---
Nitpick comments:
In `@packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts`:
- Line 17: Rename the type alias THRESHOLDS to PascalCase Thresholds and update
all references to use the new name; specifically change the declaration "type
THRESHOLDS = { abbreviationThreshold: number; decimalDigitThreshold: number }"
to "type Thresholds = {...}" and update the parameter type on the function that
currently accepts THRESHOLDS (the function signature that mentions
abbreviationThreshold/decimalDigitThreshold) to use Thresholds instead; ensure
any imports/exports and tests referencing THRESHOLDS are updated to Thresholds
as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d7950e02-5f51-4611-84ce-47ba875cc365
📒 Files selected for processing (1)
packages/javascript/bh-shared-ui/src/utils/numberFormatting.ts
1 participant
Description
This PR Improves rounding in the Posture page, preventing rounding values below 100k.
Screen.Recording.2026-03-09.at.3.10.23.PM.mov
Motivation and Context
Given a system has more than 999 findings, attack paths, or objects the selected zone
When the user views the Posture page
Then they see up to 5 digits before the product rounds the numbers
Notes:
This applies to the following fields:
Findings and Change in the Attack Paths Table
Attack Paths, Findings, and T0 (or other zones) Objects in the Summary section
Once numbers surpass 99,999, follow this rounding format:
xxxK
y.yyyM
z.zzzB
use standard rounding (x<5 then round down, x≥5 then round up to nearest next unit.
Resolves BED-7595
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests