storage: show sizes in binary units and align tests

[KKoukiou]

Add formatBytes() with base2 sizing matching blivet and Anaconda Backend. Update integration test expectations to MiB/GiB and use EFI_PARTITION_DEFAULT_SIZE_SPACE (500 MiB) for default EFI rows.
Note: cockpit-storaged still uses decimal units

[gemini-code-assist]

Code Review

This pull request migrates the storage UI and its corresponding tests from decimal units (GB/MB) to binary units (GiB/MiB) to match the underlying blivet formatting. It introduces a new formatBytes helper wrapping cockpit.format_bytes with base2: true by default, updates unit multipliers, and replaces direct calls across several React components. The feedback recommends adding a defensive guard in formatBytes to handle null, undefined, or NaN values gracefully (especially when returning separate values), and correcting the PARTITION_1GB_SIZE test constant to prevent sfdisk parsing errors caused by an unexpected space.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The formatBytes wrapper’s overload detection based on secondArg/thirdArg being plain objects is a bit opaque; consider documenting the expected cockpit.format_bytes signatures or adding small helper overloads to make the call paths and option handling more explicit and less error‑prone.
• The PARTITION_1GB_SIZE and PARTITION_1GB_SIZE_SPACE constants both being set to '954 MiB' is confusing given the comment about ~1 GB (decimal); consider renaming or adjusting values so the distinction between input size and UI‑shown size is clearer.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `formatBytes` wrapper’s overload detection based on `secondArg`/`thirdArg` being plain objects is a bit opaque; consider documenting the expected cockpit.format_bytes signatures or adding small helper overloads to make the call paths and option handling more explicit and less error‑prone.
- The `PARTITION_1GB_SIZE` and `PARTITION_1GB_SIZE_SPACE` constants both being set to `'954 MiB'` is confusing given the comment about ~1 GB (decimal); consider renaming or adjusting values so the distinction between input size and UI‑shown size is clearer.

## Individual Comments

### Comment 1
<location path="src/components/storage/installation-method/ReclaimSpaceModal.jsx" line_range="463-464" />
<code_context>
     const [value, setValue] = useState(device.total.v);
-    const originalValue = cockpit.format_bytes(device.total.v, { separate: true })[0];
-    const originalUnit = cockpit.format_bytes(device.total.v, { separate: true })[1];
+    const originalValue = formatBytes(device.total.v, { separate: true })[0];
+    const originalUnit = formatBytes(device.total.v, { separate: true })[1];
     const [inputValue, setInputValue] = useState(originalValue);

</code_context>
<issue_to_address>
**suggestion:** Repeated `formatBytes(..., { separate: true })` calls can be consolidated to avoid duplicate work and keep `originalValue`/`originalUnit` in sync.

`formatBytes(device.total.v, { separate: true })` is invoked multiple times for the same value (e.g. when initializing state and in some blur/error paths), duplicating work and risking divergence if the formatter’s behavior ever changes.

Consider calling it once, destructuring `[originalValue, originalUnit]`, and reusing those for the initial device size. You’d still call `formatBytes` for derived values like `value` or `newValue`, but the initial case doesn’t need repeated calls and becomes easier to follow.

Suggested implementation:

```javascript
    const shrinkButtonTooltipId = idPrefix + "-shrink-tooltip-" + device["device-id"].v;
    const [isPopoverOpen, setIsPopoverOpen] = useState(false);
    const [value, setValue] = useState(device.total.v);
    const [originalValue, originalUnit] = formatBytes(device.total.v, { separate: true });
    const [inputValue, setInputValue] = useState(originalValue);

```

Search within `ReclaimSpaceModal.jsx` for other occurrences of `formatBytes(device.total.v, { separate: true })`. If any of them are meant to re-derive the original device size (for example in blur handlers or error/reset paths), replace those calls with the already computed `originalValue`/`originalUnit` pair instead of recomputing. Keep separate calls for true derived values (e.g. using `value`, `newValue`, etc.), as those should still be recalculated based on the current state.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The formatBytes helper relies on positional ...args handling and isPlainObject checks, which makes its call patterns a bit opaque; consider mirroring the cockpit.format_bytes(number, unit?, options?) signature explicitly with named parameters to improve readability and reduce the risk of misusing the API.
• The unitMultiplier map now only covers B/KiB/MiB/GiB/TiB; since it is keyed off the unit strings coming from formatBytes, it may be safer to either guard against unknown units (e.g. fallback or error) or document/validate that formatBytes(..., { separate: true }) will never return other units like PiB.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `formatBytes` helper relies on positional `...args` handling and `isPlainObject` checks, which makes its call patterns a bit opaque; consider mirroring the `cockpit.format_bytes(number, unit?, options?)` signature explicitly with named parameters to improve readability and reduce the risk of misusing the API.
- The `unitMultiplier` map now only covers B/KiB/MiB/GiB/TiB; since it is keyed off the unit strings coming from `formatBytes`, it may be safer to either guard against unknown units (e.g. fallback or error) or document/validate that `formatBytes(..., { separate: true })` will never return other units like PiB.

## Individual Comments

### Comment 1
<location path="src/helpers/storage.js" line_range="191" />
<code_context>
+ * Format a byte count (binary KiB/MiB/GiB).
+ * Wraps cockpit.format_bytes with base2: true by default.
+ */
+export const formatBytes = (number, ...args) => {
+    if (args.length === 0) {
+        return cockpit.format_bytes(number, { base2: true });
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new formatBytes helper by using explicit parameters and a simple type check on the second argument instead of a variadic args array plus isPlainObject branching.

You can keep all existing call patterns while simplifying the helper and removing the runtime “mini type system”.

Instead of `...args` + `isPlainObject`, make the signature explicit and branch only on whether the second arg is a string (unit) or an options object:

```js
/**
 * Format a byte count (binary KiB/MiB/GiB).
 * Wraps cockpit.format_bytes with base2: true by default.
 */
export const formatBytes = (number, unitOrOptions, maybeOptions) => {
    if (typeof unitOrOptions === "string") {
        // formatBytes(number, unit)
        // formatBytes(number, unit, options)
        const options = { base2: true, ...(maybeOptions || {}) };
        return cockpit.format_bytes(number, unitOrOptions, options);
    }

    // formatBytes(number)
    // formatBytes(number, options)
    const options = { base2: true, ...(unitOrOptions || {}) };
    return cockpit.format_bytes(number, options);
};
```

This retains:

- `formatBytes(number)` → `cockpit.format_bytes(number, { base2: true })`
- `formatBytes(number, { ... })` → `cockpit.format_bytes(number, { base2: true, ... })`
- `formatBytes(number, "MiB")` → `cockpit.format_bytes(number, "MiB", { base2: true })`
- `formatBytes(number, "MiB", { ... })` → `cockpit.format_bytes(number, "MiB", { base2: true, ... })`

while removing:

- `isPlainObject`
- `...args` and `args.length` checks
- the extra branching complexity.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (complexity): Consider simplifying the new formatBytes helper by using explicit parameters and a simple type check on the second argument instead of a variadic args array plus isPlainObject branching.

You can keep all existing call patterns while simplifying the helper and removing the runtime “mini type system”.

Instead of ...args + isPlainObject, make the signature explicit and branch only on whether the second arg is a string (unit) or an options object:

/**
 * Format a byte count (binary KiB/MiB/GiB).
 * Wraps cockpit.format_bytes with base2: true by default.
 */
export const formatBytes = (number, unitOrOptions, maybeOptions) => {
    if (typeof unitOrOptions === "string") {
        // formatBytes(number, unit)
        // formatBytes(number, unit, options)
        const options = { base2: true, ...(maybeOptions || {}) };
        return cockpit.format_bytes(number, unitOrOptions, options);
    }

    // formatBytes(number)
    // formatBytes(number, options)
    const options = { base2: true, ...(unitOrOptions || {}) };
    return cockpit.format_bytes(number, options);
};

This retains:

• formatBytes(number) → cockpit.format_bytes(number, { base2: true })
• formatBytes(number, { ... }) → cockpit.format_bytes(number, { base2: true, ... })
• formatBytes(number, "MiB") → cockpit.format_bytes(number, "MiB", { base2: true })
• formatBytes(number, "MiB", { ... }) → cockpit.format_bytes(number, "MiB", { base2: true, ... })

while removing:

• isPlainObject
• ...args and args.length checks
• the extra branching complexity.