Skip to content

Localize cost history chart labels - #2983

Merged
steipete merged 2 commits into
steipete:mainfrom
Yuxin-Qiao:fix/chart-menu-localization
Aug 17, 2026
Merged

Localize cost history chart labels#2983
steipete merged 2 commits into
steipete:mainfrom
Yuxin-Qiao:fix/chart-menu-localization

Conversation

@Yuxin-Qiao

Copy link
Copy Markdown
Contributor

Summary

  • Localize the five hardcoded English labels in the per-provider cost history chart submenu: the Projects header, the Conversations (window) header, Session <id> rows, the Std/Fast mode prefixes on model rows, and the + N more source-overflow hint.
  • Adds the six new keys (Projects, Conversations (%@), Session %@, + %d more, Std, Fast) to all 23 app locales; English strings are unchanged, so this is a pure localization-completeness fix with no behavior change.

Commands run

  • swift build
  • swift test --filter CostHistoryChartMenuViewTests — 41 tests passed
  • node Scripts/check-app-locales.mjs — 22 catalogs OK against 1411 English keys
  • swiftformat Sources Tests / swiftlint --strict — clean
  • ./Scripts/test.sh (full sharded suite) — green after fixing the Italian Std value to Std. so the catalog's untranslated-value gate stays exact (676.9s, 0 failures)

Notes

  • Follows the existing L(...) pattern; Std/Fast are treated as translatable words (e.g. 標準/高速, Стандарт/Быстро) since they abbreviate Codex standard/priority mode for non-English readers.

@clawsweeper

clawsweeper Bot commented Aug 16, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 16, 2026
@clawsweeper

clawsweeper Bot commented Aug 16, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 16, 2026, 9:13 PM ET / August 17, 2026, 01:13 UTC.

ClawSweeper review

What this changes

The PR replaces six raw cost-history menu labels with localization lookups and adds translations in all bundled locale catalogs.

Merge readiness

Blocked until real behavior proof from a real setup is added - 4 items remain

Keep open: the focused localization fix remains needed, but it needs a small regression test and contributor-provided runtime menu proof before merge.

Priority: P3
Reviewed head: 7cccaa03e71e822061d45da7a77cbef7a9889ec7

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The code follows the established localization path, but merge confidence is capped until the app’s rendered menu is shown after the fix.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The PR reports builds, tests, catalog validation, formatting, and linting, but no after-fix runtime screenshot, recording, terminal output, or live menu evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR reports builds, tests, catalog validation, formatting, and linting, but no after-fix runtime screenshot, recording, terminal output, or live menu evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Current main still has raw labels: The shipped current-main chart code constructs the affected project, overflow, conversation, session, and mode labels as raw English strings.
Existing localization mechanism: The app provides an L helper for localized strings and formatted localized values; the PR uses that established path.
Test-policy gap: Repository policy requests focused XCTest coverage for new logic; the PR changes the view and catalogs but does not extend the existing source-level localization coverage.
Findings 1 actionable finding [P3] Cover the new localized labels
Security None None.

How this fits together

CodexBar renders provider cost-history data as a SwiftUI menu-bar chart with project and conversation detail rows. Those labels pass through the app localization helper before reaching the native menu UI.

flowchart LR
A[Provider cost history] --> B[Cost-history chart]
B --> C[Menu detail rows]
C --> D[Localization helper]
D --> E[Localized menu bar UI]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR reports builds, tests, catalog validation, formatting, and linting, but no after-fix runtime screenshot, recording, terminal output, or live menu evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Cover the new localized labels (P3) - Late review note: this view change is unchanged from the previous review, but it still lacks the focused XCTest coverage required by repository guidance. Extend the existing source-level localization coverage with the six raw-label markers so a future edit cannot silently reintroduce English-only menu text.
  • Resolve merge risk (P2) - No after-fix runtime evidence shows the populated localized menu rendering in the real app; reported tests and catalog checks are supplemental only.
  • Complete next step (P2) - The remaining merge gate is contributor-supplied real behavior proof, not an autonomous repair task.

Findings

  • [P3] Cover the new localized labels — Sources/CodexBar/CostHistoryChartMenuView.swift:326
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Localization coverage 6 labels across 23 catalogs The functional change is narrow while ensuring every bundled app language has matching keys.
Production versus test delta production +141 net lines, tests +0 The localization catalog growth is justified, but the changed UI path lacks the repository-requested focused regression coverage.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Keep the existing L-based implementation, add narrow source-level regression coverage for all six labels, and attach redacted runtime evidence of the populated localized cost-history menu.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Keep the existing L-based implementation, add narrow source-level regression coverage for all six labels, and attach redacted runtime evidence of the populated localized cost-history menu.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main still passes the affected chart labels directly as English text instead of through localization.

Is this the best way to solve the issue?

Yes, with the added regression coverage: using the existing L helper and catalogs is the narrowest maintainable repair for these labels.

Full review comments:

  • [P3] Cover the new localized labels — Sources/CodexBar/CostHistoryChartMenuView.swift:326
    Late review note: this view change is unchanged from the previous review, but it still lacks the focused XCTest coverage required by repository guidance. Extend the existing source-level localization coverage with the six raw-label markers so a future edit cannot silently reintroduce English-only menu text.
    Confidence: 0.97
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is correct
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against cfe6c925cbca.

Labels

Label justifications:

  • P3: This is a low-risk localization-completeness bug with limited user impact.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports builds, tests, catalog validation, formatting, and linting, but no after-fix runtime screenshot, recording, terminal output, or live menu evidence. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • steipete: The current cost-history chart labels date to the recent release commit and its lines are attributed to this contributor. (role: recent area contributor; confidence: high; commits: 208ae357982a; files: Sources/CodexBar/CostHistoryChartMenuView.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add focused localization-regression coverage for the six labels.
  • Attach a redacted screenshot or recording of the populated localized cost-history menu, then update the PR body to trigger re-review; if needed, ask a maintainer to comment @clawsweeper re-review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-08-16T18:39:55.337Z sha 7b1808e :: needs real behavior proof before merge. :: none

@steipete
steipete merged commit 45ca0b4 into steipete:main Aug 17, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants