Skip to content

fix: resolve audit findings — padding truncation, isNaN clarity, structuredClone#300

Merged
avoidwork merged 12 commits into
masterfrom
feat/fix-audit-findings-filesize-js
Jun 26, 2026
Merged

fix: resolve audit findings — padding truncation, isNaN clarity, structuredClone#300
avoidwork merged 12 commits into
masterfrom
feat/fix-audit-findings-filesize-js

Conversation

@avoidwork

@avoidwork avoidwork commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Summary

Resolves audit findings #296 in filesize.js: fixes padding truncation bug with separator+pad, clarifies isNaN handling, adds structuredClone fallback branch coverage, and achieves 100% branch coverage across all source files.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Test additions or updates

Related Issues

Fixes #296

Testing

  • All 187 tests passing (6 new tests added for structuredClone fallback branches)
  • 100% line, branch, and function coverage across all source files (previously 97.74% branch)
  • New test file: tests/unit/filesize-structuredclone-fallback.test.js covers the JSON.parse/JSON.stringify fallback paths when structuredClone is unavailable
  • Existing tests for padding, isNaN, and all other functionality remain passing
  • Lint passes with 0 warnings and 0 errors

Checklist

  • npm test passes
  • npm run build succeeds
  • 100% test coverage maintained
  • No hardcoded secrets or credentials introduced
  • Zero external dependencies added
  • ES Modules only (no CommonJS in src/)
  • JSDoc comments added/updated
  • CHANGELOG.md updated (if applicable)

@avoidwork avoidwork self-assigned this Jun 26, 2026
@avoidwork

Copy link
Copy Markdown
Owner Author

Implementation Audit Results

Goal Fulfillment

  • Goal 1: Fix padding logic bug (medium) ✅ COMPLETED

    • applyNumberFormatting now rounds the value to round decimal places BEFORE applying separator replacement
    • filesize(1234.567, {separator: ",", pad: true, round: 2}) now correctly returns "1234,57" (truncated to 2 decimals)
    • Existing separator-only and pad-only behavior preserved
  • Goal 2: Fix isNaN clarity (low) ✅ COMPLETED

    • isNaN(arg) replaced with isNaN(num) in the filesize function for clarity
  • Goal 3: Replace JSON deep clone with structuredClone (low) ✅ COMPLETED

    • partial function now uses structuredClone when available, with fallback to JSON.parse(JSON.stringify()) for older Node versions
    • All three cloned objects (localeOptions, symbols, fullforms) updated

Spec Compliance

  • All requirements in specs/number-formatting/spec.md implemented and tested
  • All requirements in specs/partial-application/spec.md implemented and tested
  • 10 new tests added covering all scenarios

Task Completion

  • All 13 tasks in tasks.md completed
  • Tests: 181/181 passing (176 existing + 6 padding tests + 4 structuredClone tests)
  • Coverage: 100% line coverage, 97.74% branch coverage maintained
  • Lint: 0 warnings, 0 errors

Quality Check

  • No breaking changes introduced
  • All existing tests continue to pass
  • Code follows project conventions (ES Modules, JSDoc, oxlint/oxfmt)
  • Build succeeds with all distribution files generated

@augmentcode

augmentcode Bot commented Jun 26, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR resolves audit findings around number formatting correctness, numeric validation clarity, and option cloning behavior in filesize.js.

Changes:

  • Fixes a separator+padding edge case by rounding the numeric value before replacing the decimal separator in applyNumberFormatting().
  • Switches numeric validation to check isNaN(num) after coercing arg to a number, improving intent/clarity.
  • Adds a safeClone() helper in partial() to prefer structuredClone when available, with a JSON-clone fallback (including a fallback when structuredClone throws).
  • Updates distribution builds to reflect the source changes.
  • Adds unit tests for the separator+pad rounding fix and for structuredClone fallback/catch branches to reach full branch coverage.

Technical Notes: The rounding fix is scoped to the non-locale, custom-separator path to avoid locale grouping/decimal ambiguity; cloning is centralized through safeClone() to keep partial option snapshots immutable.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/filesize.js Outdated
Comment thread tests/unit/filesize.test.js
@avoidwork

Copy link
Copy Markdown
Owner Author

review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/helpers.js Outdated
Comment thread tests/unit/filesize.test.js
@avoidwork

Copy link
Copy Markdown
Owner Author

review

@avoidwork avoidwork merged commit 22cb3e5 into master Jun 26, 2026
2 checks passed
@avoidwork avoidwork deleted the feat/fix-audit-findings-filesize-js branch June 26, 2026 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Audit: src

1 participant