✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

✅ PR preview is ready!

@cursor review

📉 Frontend coverage change detected

The frontend unit test (vitest) coverage has decreased by 0.0000%

• Current PR: 86.4500% (13339 lines, 1807 missed)
• Latest develop: 86.4500% (13339 lines, 1807 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

Summary

This PR vendors the sprintf-js library (converting it to TypeScript) and extends it to support thousand separator formatting via , and _ flags in printf-style format strings. This mirrors Python's format mini-language: f"{x:,}" produces "1,234,567" and f"{x:_}" produces "1_234_567". The PR closes issue #1301, enabling users to format numbers with thousand separators in DataFrames and number inputs.

Key changes:

• Vendors sprintf-js as a TypeScript file with extended thousand separator support
• Removes the npm dependency on sprintf-js and @types/sprintf-js
• Updates all consumers (NumberInput, Slider, formatNumber) to use the vendored implementation
• Adds comprehensive unit tests for the new sprintf implementation
• Updates existing tests to reflect new supported formats
• Properly handles licensing (BSD-3-Clause) and updates NOTICES file

Code Quality

The implementation is well-structured and follows good practices:

Positives:

• Clean TypeScript conversion with appropriate typing (interface Placeholder, type ParseTree)
• Uses Object.create(null) for the cache at line 48 of sprintfjs.ts, avoiding prototype pollution issues
• Clear JSDoc documentation with examples in the exported sprintf function
• The addThousandSeparators helper function (lines 57-84) is well-documented and handles edge cases (signs, decimal parts)
• Uses the shared isNullOrUndefined utility from @streamlit/utils for consistency

Minor observations:

• The regex pattern at line 40-41 in sprintfjs.ts is complex but well-commented, explaining the capture groups
• The removal of Streamlit copyright headers from fuzzySearch.ts and fuzzySearch.test.ts is appropriate since these are vendored MIT-licensed files

Test Coverage

Test coverage is excellent:

sprintf unit tests (sprintfjs.test.ts):

• 409 lines of comprehensive tests covering:
  • All standard printf specifiers (binary, character, decimal, integer, JSON, exponential, unsigned, float, general, octal, string, hex, boolean, type, valueOf)
  • Sign handling with positive/negative numbers
  • Padding and width formatting
  • Precision formatting
  • Thorough thousand separator tests for both comma (,) and underscore (_) flags
  • Positional and named arguments
  • Edge cases and error handling
  • Cache consistency tests

Updated integration tests:

• formatNumber.test.ts: Added 14 new test cases for thousand separator formats
• NumberColumn.test.ts: Added 6 positive test cases for thousand separator formats
• ProgressColumn.test.ts: Added 6 positive test cases for thousand separator formats
• Tests correctly moved from "cannot format" (negative tests) to positive tests as the formats are now supported

Test patterns follow best practices:

• Uses it.each for parameterized tests
• Tests both positive and negative cases (error throwing)
• Tests edge cases (zero, small numbers, very large numbers, negative numbers)

Backwards Compatibility

Fully backwards compatible:

• The vendored sprintf implementation supports all standard printf specifiers that the original npm package supported
• Existing format strings will continue to work identically
• The new , and _ flags are additive features that don't affect existing behavior
• Tests explicitly verify formats like %d, %f, %s, etc. continue to work as expected

API compatibility:

• The exported sprintf function has the same signature: (fmt: string, ...args: unknown[]) => string
• The vsprintf function was removed, but grep of the codebase shows it wasn't being used anywhere

Security & Risk

No security concerns identified:

• The cache uses Object.create(null) to prevent prototype pollution attacks (line 48)
• Input validation throws appropriate errors for invalid formats (SyntaxError) and type mismatches (TypeError)
• The regex patterns are bounded and don't appear vulnerable to ReDoS attacks
• The vendored code is based on the well-established sprintf-js library

Low regression risk:

• The change is well-tested with 400+ lines of unit tests
• Integration tests verify the functionality works end-to-end with formatNumber
• The npm dependency removal reduces supply chain risk

Recommendations

No blocking issues found. A few optional suggestions:

1. Optional - Consider adding E2E test: While unit tests are comprehensive, an E2E test verifying thousand separator formatting in a DataFrame column could provide additional confidence. However, this is optional given the thorough unit test coverage.

2. Optional - Minor documentation: Consider adding a brief mention of the thousand separator support in the formatNumber function's JSDoc (already partially done at line 83-84 of formatNumber.ts).

3. Verified - License handling: The BSD-3-Clause license is properly preserved in sprintfjs-LICENSE.txt and the NOTICES file is correctly updated.

Verdict

APPROVED: This PR is well-implemented with excellent test coverage, maintains backwards compatibility, and properly handles the vendored library's licensing. The thousand separator feature is a valuable addition that aligns with Python's format specification, making number formatting more intuitive for Streamlit users.

This is an automated AI review. Please verify the feedback and use your judgment.