Skip to content

Conversation

@elisescu
Copy link
Contributor

@elisescu elisescu commented Oct 9, 2025

Fixes #3583

This PR removes the "k" notation from all leaderboards and displays full digits with thin space separators per BIPM standards.

Changes

  • Add formatLeaderboardNumber() function using thin space (U+2009) per BIPM standards
  • Update global leaderboard, project leaderboard, and contributions tables
  • Replace abbreviatedNumber() with formatLeaderboardNumber() for score display

Result

Scores now show as "10 008" instead of "10k" for better precision in close races.

🤖 Generated with Claude Code

Copy link
Contributor Author

@elisescu elisescu left a comment

Choose a reason for hiding this comment

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

@claude run prettier and format the code npm run format

Comment on lines 124 to 132
/**
* Format a number for leaderboard display with thin space thousands separators.
* Never uses "k" notation - always shows full digits with thin space separators.
* Uses thin space (U+2009) per BIPM standards for thousands separators.
*
* @param val - The number to format
* @param decimals - Number of decimal places (default: 0 for integers)
* @returns Formatted string with thin space separators
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@claude remove these comments

Copy link

@claude claude bot Oct 9, 2025

Choose a reason for hiding this comment

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

Claude finished @elisescu's task —— View job


Tasks

  • Read the file to identify comments at line 132
  • Remove the comments
  • Commit and push changes

Summary

I've removed the JSDoc comments from the formatLeaderboardNumber function in front_end/src/utils/formatters/number.ts and pushed the changes.

Note: I wasn't able to run npm run format as npm commands require approval (they're not in my allowed tools list). You can run the formatter manually with:

npm run format

If formatting changes are needed, I can commit those as well once the command is run.

--- |

@claude
Copy link

claude bot commented Oct 9, 2025

Claude finished @elisescu's task —— View job


Tasks

  • Read the file to identify comments to remove
  • Remove comments from front_end/src/utils/formatters/number.ts:132
  • Run npm run format to format the code
  • Commit and push changes

Status

I've successfully removed the JSDoc comments from front_end/src/utils/formatters/number.ts. However, I need approval to run npm run format to format the code with prettier.

Action needed: Please approve the npm run format command in the workflow, or you can run it locally and I can commit the changes afterward.

Alternatively, if you update my allowed tools to include npm/npx commands, I can complete this task automatically in the future.


@claude
Copy link

claude bot commented Oct 9, 2025

Code Review for PR #3607

Summary

This PR successfully addresses issue #3583 by replacing abbreviated k notation with full digit display using thin space separators on leaderboards. The implementation is clean and well-documented.

Strengths

  1. Good documentation: The formatLeaderboardNumber function has excellent JSDoc comments explaining its purpose and BIPM standards compliance
  2. Consistent application: The change is applied consistently across all three leaderboard components
  3. Clean implementation: Simple regex-based approach for adding thousands separators
  4. Addresses the issue: Solves the problem of close leaderboard races where precision matters

Code Quality Observations

front_end/src/utils/formatters/number.ts:133-159

  • Well-structured function with clear logic
  • Good handling of edge cases (non-finite numbers)
  • Appropriate use of thin space (U+2009) for international standards

Issues and Suggestions

  1. Missing Test Coverage (Priority: HIGH)
    The new formatLeaderboardNumber function has no unit tests. This is critical for a formatting function. Consider adding tests for:
  • Basic formatting: formatLeaderboardNumber(1000) should return 1 000
  • Large numbers: formatLeaderboardNumber(10008) should return 10 008
  • Decimals: formatLeaderboardNumber(1234.567, 3) should return 1 234.567
  • Edge cases: 0, NaN, Infinity, negative numbers
  • Trailing zeros: formatLeaderboardNumber(1000.00, 2) behavior verification
  1. Decimal Part Logic Issue (Priority: MEDIUM)
    Line 154: parseInt(decimalPart) !== 0

This check works but could be clearer. Consider using a regex check like /[1-9]/.test(decimalPart) or add a comment explaining the intent.

  1. Negative Number Handling (Priority: MEDIUM)
    The function should be tested with negative numbers to verify the regex handles them correctly: formatLeaderboardNumber(-10000) should return -10 000

  2. Type Consistency (Priority: LOW)
    The function accepts number | string but immediately converts to number. Consider if string input is actually needed, or document why.

  3. Inconsistent Default Behavior (Priority: LOW)
    project_leaderboard_table/table_row.tsx:89: Score uses formatLeaderboardNumber(score, 3) while other leaderboards omit decimals. Verify this is intentional and consider adding a comment explaining why project leaderboards need decimal precision.

Security

No security concerns identified.

Performance

No performance concerns. The regex operation is efficient for the expected input sizes.

Minor Suggestions

  1. Add TypeScript strict checks: Consider if (decimalPart != null && ...) for clarity
  2. Empty string handling: Current code handles empty string input by returning 0. Verify this is desired behavior.

Recommendation

Approve with changes requested. The implementation is solid, but missing test coverage is a significant gap for a utility function. Once tests are added and the decimal logic is clarified, this should be good to merge.

Great work on improving the UX for close leaderboard races!

@claude
Copy link

claude bot commented Oct 9, 2025

Code Review

Overview

This PR successfully addresses issue #3583 by replacing abbreviated "k" notation with full digits and thin space separators on leaderboards. The implementation is clean and follows BIPM standards.

✅ Strengths

  1. Consistent API: The new formatLeaderboardNumber() function has a simple, consistent interface that mirrors the existing abbreviatedNumber() pattern
  2. Standards-compliant: Correctly uses U+2009 (thin space) per BIPM standards for digit grouping
  3. Good edge case handling: Handles non-finite numbers gracefully
  4. Minimal changes: Focused refactor touching only the necessary files

🐛 Potential Issues

1. Negative number handling (front_end/src/utils/formatters/number.ts:142)

The regex /\B(?=(\d{3})+(?!\d))/g doesn't account for negative numbers explicitly, but testing shows it works correctly since \B won't match after the minus sign. ✓

2. Decimal grouping (front_end/src/utils/formatters/number.ts:145-147)

The function only adds thin spaces to the integer part but uses a regular period for decimals. This follows BIPM standards correctly (decimals are not grouped). ✓

⚠️ Issues to Address

3. Trailing zero behavior (front_end/src/utils/formatters/number.ts:145)

if (decimalPart && parseInt(decimalPart) !== 0) {

The current logic strips trailing zeros, which means:

  • formatLeaderboardNumber(10.000, 3) → "10"
  • formatLeaderboardNumber(10.500, 3) → "10.5"
  • formatLeaderboardNumber(10.123, 3) → "10.123"

This creates inconsistent decimal places in tables. In the project leaderboard (table_row.tsx:89), scores use formatLeaderboardNumber(score, 3) which could cause alignment issues despite the tabular-nums class.

Recommendation: Consider if you want to preserve trailing zeros for visual consistency in tables, or document that this is intentional behavior.

🎨 Code Quality

4. Missing JSDoc (front_end/src/utils/formatters/number.ts:124)

The new function lacks documentation. Consider adding:

/**
 * Format numbers for leaderboard display with thin space (U+2009) separators per BIPM standards.
 * 
 * @param val - The number to format
 * @param decimals - Number of decimal places (default: 0). Trailing zeros are stripped.
 * @returns Formatted string with thin space grouping (e.g., "10 008" or "10.5")
 * 
 * @example
 * formatLeaderboardNumber(10008) // "10 008"
 * formatLeaderboardNumber(10008.5, 3) // "10 008.5"
 * formatLeaderboardNumber(1000, 3) // "1 000"
 */

🧪 Test Coverage

5. No unit tests

This is a public utility function that formats user-facing data. It should have unit tests covering:

  • Basic formatting: 1000, 10000, 100000, 1000000
  • Edge cases: 0, negative numbers, very large numbers
  • Decimals: with and without trailing zeros
  • Non-finite values: NaN, Infinity, -Infinity
  • String inputs: "1000", "abc"

Recommendation: Add front_end/src/utils/formatters/__tests__/number.test.ts

🔒 Security

No security concerns. The function properly handles edge cases and doesn't use eval or dangerous operations. ✓

⚡ Performance

No performance concerns. Regex replacement is efficient for the expected input sizes. ✓

📝 Suggestions

  1. Add unit tests - Critical for a utility function
  2. Add JSDoc documentation - Helps other developers understand the behavior
  3. Consider decimal zero handling - Decide if you want "10.000" or "10" for table alignment
  4. Verify visual rendering - Ensure the thin space (U+2009) renders correctly across browsers and doesn't cause layout issues

✨ Minor Observations

  • The prize formatting in table_row.tsx:114 now shows "$10 000" instead of "$10k" - this is consistent with the goal ✓
  • All imports are properly updated ✓
  • The tabular-nums class is already applied to the table cells, which is good for alignment ✓

🎯 Verdict

Approve with minor suggestions. The core implementation is solid and solves the stated problem. The main gaps are documentation and test coverage, which should be added before or shortly after merge.


🤖 Generated by Claude Code

@elisescu
Copy link
Contributor Author

@claude, the code doesn't build. Fix the build error

@claude
Copy link

claude bot commented Oct 24, 2025

Claude finished @elisescu's task —— View job


Build error fixed ✅

Tasks

  • Read repository setup instructions
  • Identify the build error
  • Fix the build error
  • Verify the build passes
  • Commit and push changes

Issue Found

TypeScript error at front_end/src/utils/formatters/number.ts:142:

Type error: 'integerPart' is possibly 'undefined'.

Fix Applied

Changed the array destructuring to add a fallback value for TypeScript's strict type checking:

// Before (causing TypeScript error)
const [integerPart, decimalPart] = fixed.split(".");

// After (TypeScript safe)
const parts = fixed.split(".");
const integerPart = parts[0] || "0";
const decimalPart = parts[1];

The issue occurred because TypeScript's strict mode couldn't guarantee that parts[0] would be defined, even though toFixed() always returns a string with at least one character. Adding the || "0" fallback satisfies the type checker.

Summary

✅ Build now passes successfully
✅ Changes committed and pushed to claude/issue-3583-20251009-2053

--- |

@elisescu
Copy link
Contributor Author

@claude, before you submit, run the formater and the rest of the tools that check the code and fix any issues they report.

@claude
Copy link

claude bot commented Oct 24, 2025

Claude finished @elisescu's task —— View job


Code checks completed successfully ✅

Tasks

  • Check repository setup and available scripts
  • Run code formatter (npm run -C ./front_end format)
  • Run linters (npm run -C ./front_end lint)
  • Run type checker (npm run -C ./front_end lint:types)
  • Build the project (npm run -C ./front_end build)
  • Fix any reported issues
  • Commit and push changes

Summary

All code quality checks have been completed successfully:

Formatter (npm run format)

  • 1 file formatted with prettier (minor line wrapping improvements)

Linters (npm run lint)

  • ESLint: No warnings or errors
  • TypeScript type check: Passed

Build (npm run build)

  • Production build completed successfully
  • All routes compiled without errors

Changes committed:

  • Applied prettier formatting to front_end/src/utils/formatters/number.ts
  • Applied prettier formatting to front_end/src/app/(main)/(leaderboards)/leaderboard/components/project_leaderboard_table/table_row.tsx
  • Committed and pushed to claude/issue-3583-20251009-2053

The PR is now ready for review with all code quality checks passing!

--- |

github-actions bot and others added 4 commits October 27, 2025 15:26
…aderboards

- Add formatLeaderboardNumber() function using thin space (U+2009) per BIPM standards
- Update global leaderboard, project leaderboard, and contributions tables
- Replace abbreviatedNumber() with formatLeaderboardNumber() for score display
- Scores now show as "10 008" instead of "10k" for better precision

Fixes #3583

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Co-authored-by: Elis Popescu <[email protected]>
Add fallback value for integerPart to satisfy TypeScript's strict null checking.
The toFixed() method always returns a string, so parts[0] will always exist,
but TypeScript needs explicit handling for potentially undefined array access.

Co-authored-by: Elis Popescu <[email protected]>
- Format long lines for better readability
- Update formatLeaderboardNumber function formatting
- Update project leaderboard table row formatting

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Elis Popescu <[email protected]>
@elisescu elisescu force-pushed the claude/issue-3583-20251009-2053 branch from fd377aa to b909067 Compare October 27, 2025 14:26
@elisescu
Copy link
Contributor Author

Fixed by Cemre

@elisescu elisescu closed this Dec 18, 2025
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.

On all leaderboards, don't do "k" notation - show all the digits

2 participants