Skip to content

fix(desktop): make English number formatting deterministic - #71750

Closed
yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix/desktop-deterministic-us-formatting
Closed

yinkev wants to merge 1 commit into
NousResearch:mainfrom
yinkev:fix/desktop-deterministic-us-formatting

Conversation

@yinkev

@yinkev yinkev commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes the Desktop's hard-coded English number and USD strings deterministic across host locales.

On native Windows with an en-DE ICU locale, the current formatters produce 5.000 and US$25, while the surrounding English copy, existing billing formatter, and tests require 5,000 and $25. This pins the three outlier formatting sites to en-US, matching the explicit contract already used by use-billing-state.ts for USD display strings.

The change is intentionally scoped to copy that is currently hard-coded English. It does not change user-locale-aware UI formatting elsewhere.

Related Issue

Fixes #71659

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/components/assistant-ui/tool/fallback-model/format.ts
    • format the hard-coded English truncation count with en-US grouping.
  • apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts
    • format usage call/token counts with stable en-US grouping.
  • apps/desktop/src/app/settings/billing/billing-amounts.ts
    • use the same en-US USD contract as the existing billing-state formatter.
  • focused regressions
    • assert the two English number paths pass en-US explicitly;
    • simulate a non-US default locale for the billing formatter and require $25.

How to Test

  1. Install the locked JavaScript dependencies with npm ci --ignore-scripts.
  2. Run the Desktop type checker:
    cd apps/desktop
    npm run typecheck
  3. Run the focused locale regressions:
    npm run test:ui -- --run \
      src/components/assistant-ui/tool/fallback-model.test.ts \
      src/app/session/hooks/use-prompt-actions/utils.test.ts \
      src/app/settings/billing/billing-amounts.test.ts
    Expected: 62 passed.
  4. Run ESLint on the six changed TypeScript files and git diff --check.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — N/A for this Desktop-only TypeScript change; focused UI tests and the complete Desktop typecheck pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS Apple Silicon, with the host-locale behavior reproduced through adversarial formatter mocks

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; no user-facing configuration or workflow changes
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — this removes host-locale variance across supported platforms
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

No visual layout changes. Focused result:

Test Files  3 passed (3)
Tests       62 passed (62)

Signed-off-by: Kevin Yin <182213728+yinkev@users.noreply.github.com>
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage area/billing Account usage, credit usage, billing (cross-cutting) sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 26, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused cross-locale regression fix. Current main still uses host-default formatting in the three reported paths: apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts:301, apps/desktop/src/components/assistant-ui/tool/fallback-model/format.ts:58, and apps/desktop/src/app/settings/billing/billing-amounts.ts:117. Pinning these hard-coded English/USD strings to en-US matches the established billing contract in apps/desktop/src/app/settings/billing/use-billing-state.ts:825-832.

Current-main changes since the PR base affect earlier helpers in utils.ts, not the usage formatter; the patch should be straightforward to salvage.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Summary

One PR addresses #71659. #71750 removes host-locale dependence from all three reported hard-coded English/USD formatting paths and adds focused regression coverage.

Related pull requests

  • fix(desktop): make English number formatting deterministic #71750 best fix — (+53/-5) — n/a: Pins usage counts, truncation counts, and USD billing amounts to en-US, directly covering every reported failure; the keep_open review confirms that current main remains affected at those sites and that the patch is highly salvageable.

Suggested consolidation

Keep #71750 open with a salvage path: preserve its three production locale fixes and focused tests, rebasing them onto current main if necessary. This follows the keep_open review, which reports that intervening changes do not affect the usage formatter; there are no competing PRs to close as duplicates.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I71659(["issue #71659 (open)"])
    P71750["PR #71750 (open)"]
    P71750 -->|best fix| I71659
    class I71659 open
    class P71750 open
    class P71750 best
    class P71750 target
    click I71659 "https://github.com/NousResearch/hermes-agent/issues/71659"
    click P71750 "https://github.com/NousResearch/hermes-agent/pull/71750"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 6 kB of PR diffs, 10 kB of issue/PR text, 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@OutThisLife

Copy link
Copy Markdown
Contributor

Superseded by #120267. The replacement fixes the reported locale-dependent test failures while preserving runtime number and currency formatting, following the test-only approach already landed in #81965. Thanks @yinkev for identifying the formatter paths and supplying regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/billing Account usage, credit usage, billing (cross-cutting) comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop UI tests and billing output depend on host locale (en-DE produces deterministic failures)

5 participants