feat: i18n Phase 1-4b wiring + rebase compile fixes#2239
Conversation
Hmbown#1599) \033[>1u enables the kitty keyboard protocol with DISAMBIGUATE_ESCAPE_CODES. On Windows Terminal, this causes Enter to emit \033[57414u, but crossterm does not decode CSI u sequences on Windows (issue Hmbown#1599). The raw string [57414u appears as text input instead of sending the message. \033[>0u advertises protocol awareness without enabling any flags. Enter stays as \r\n and crossterm processes it normally. Changes: - push_keyboard_enhancement_flags: write \033[>0u instead of \033[>1u on Windows - Gate KeyboardEnhancementFlags import behind #[cfg(not(windows))] - Update Windows test to expect \033[>0u - Non-Windows test unchanged (still expects \033[>1u) - Remove unused KeyEvent, MouseButton, MouseEvent, MouseEventKind imports introduced by upstream during rebase
…and pending input preview (Hmbown#790) Add 23 new MessageId variants with translations for all 4 shipped locales (en, ja, zh-Hans, pt-BR) covering high-frequency TUI chrome: - Composer submit/queue/steer/offline hints and queue count - Config modal section labels - Status line picker title, instruction, and action hints - Pending input preview headers and edit hint Co-Authored-By: DeepSeek V4 Flash
…wn#790) Add 41 new MessageId variants with translations for all 4 shipped locales (en, ja, zh-Hans, pt-BR) covering: - Approval field labels (Type, About, Impact, Params) - Risk badges (REVIEW, DESTRUCTIVE) and category labels - Approval option labels and destructive confirmation footer - Sandbox elevation title, option labels, descriptions, and impact text Co-Authored-By: DeepSeek V4 Flash
…mand output (Hmbown#790) Co-Authored-By: DeepSeek V4 Flash
…lized prefix (Hmbown#790) All 111+ CommandResult::error() callers across 21 command files now use the localized `error(msg, locale)` signature, replacing the hardcoded "Error:" prefix with locale-aware output via CmdErrorPrefix. Breaking helper API: parse_index, clear, parse_add, trust_add, trust_remove now take a locale: Locale parameter threaded from their callers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…anout summaries, and sub-agent modal (Hmbown#790) Co-Authored-By: Deepseek V4 Flash
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive localization (i18n) across the TUI commands, views, and widgets by replacing hardcoded English strings with localized messages retrieved via localization::tr using MessageId and the active ui_locale. The review feedback highlights several important improvements for robustness and consistency. Specifically, the reviewer points out that sequential positional placeholder replacements are fragile for internationalization and recommends named placeholders instead. Additionally, the reviewer notes that the error_msg helper documentation is misleading and contains an unnecessary #[allow(dead_code)] attribute. Finally, they advise using CommandResult::error_msg instead of CommandResult::error for usage instructions to avoid prepending an inappropriate 'Error:' prefix.
| let tmpl_transcript = tr(locale, MessageId::CtxInspectorTranscript); | ||
| let _ = writeln!( | ||
| out, | ||
| "Transcript: {} cells, {} API messages", | ||
| app.history.len(), | ||
| app.api_messages.len() | ||
| "{}", | ||
| tmpl_transcript | ||
| .replacen("{}", &app.history.len().to_string(), 1) | ||
| .replace("{}", &app.api_messages.len().to_string()) | ||
| ); |
There was a problem hiding this comment.
Using positional placeholders {} and replacing them sequentially with .replacen() and .replace() is fragile for internationalization (i18n). If a translator changes the order of the placeholders to fit the target language's grammar, the values will be swapped. It is highly recommended to use named placeholders (e.g., {cells} and {messages}) and replace them explicitly, which is already a common pattern in this codebase.
| /// Create a simple error message (English prefix) | ||
| #[allow(dead_code)] | ||
| pub fn error_msg(msg: impl Into<String>) -> Self { | ||
| Self { | ||
| message: Some(format!("Error: {}", msg.into())), | ||
| message: Some(msg.into()), | ||
| action: None, | ||
| is_error: true, | ||
| } | ||
| } |
There was a problem hiding this comment.
The error_msg function does not prepend any prefix to the message, so the comment (English prefix) is misleading. Additionally, since error_msg is widely used across multiple command files, the #[allow(dead_code)] attribute is unnecessary and should be removed.
/// Create a simple error message (no prefix)
pub fn error_msg(msg: impl Into<String>) -> Self {
Self {
message: Some(msg.into()),
action: None,
is_error: true,
}
}| let Some(raw) = arg.map(str::trim) else { | ||
| return CommandResult::error( | ||
| "Usage: /cycle <n> — n is the cycle number from /cycles".to_string(), | ||
| app.ui_locale, | ||
| ); | ||
| }; | ||
| if raw.is_empty() { | ||
| return CommandResult::error("Usage: /cycle <n>".to_string()); | ||
| return CommandResult::error("Usage: /cycle <n>".to_string(), app.ui_locale); | ||
| } |
There was a problem hiding this comment.
Using CommandResult::error for usage instructions prepends the localized Error: prefix, resulting in messages like Error: Usage: /cycle <n>. To keep usage instructions clean and prefix-free, use CommandResult::error_msg instead.
| let Some(raw) = arg.map(str::trim) else { | |
| return CommandResult::error( | |
| "Usage: /cycle <n> — n is the cycle number from /cycles".to_string(), | |
| app.ui_locale, | |
| ); | |
| }; | |
| if raw.is_empty() { | |
| return CommandResult::error("Usage: /cycle <n>".to_string()); | |
| return CommandResult::error("Usage: /cycle <n>".to_string(), app.ui_locale); | |
| } | |
| let Some(raw) = arg.map(str::trim) else { | |
| return CommandResult::error_msg("Usage: /cycle <n> — n is the cycle number from /cycles"); | |
| }; | |
| if raw.is_empty() { | |
| return CommandResult::error_msg("Usage: /cycle <n>"); | |
| } |
| _ => CommandResult::error( | ||
| localization::tr(app.ui_locale, MessageId::CmdQueueUsage), | ||
| app.ui_locale, | ||
| ), |
There was a problem hiding this comment.
Using CommandResult::error for usage instructions prepends the localized Error: prefix, even when the usage message itself is localized. Use CommandResult::error_msg instead to display the localized usage message without the error prefix.
_ => CommandResult::error_msg(
localization::tr(app.ui_locale, MessageId::CmdQueueUsage),
),| CommandResult::action(AppAction::TaskCancel { id: id.to_string() }) | ||
| } | ||
| _ => CommandResult::error("Usage: /task [add <prompt>|list|show <id>|cancel <id>]"), | ||
| _ => CommandResult::error(t(MessageId::CmdTaskUsageGeneral), app.ui_locale), |
There was a problem hiding this comment.
Using CommandResult::error for usage instructions prepends the localized Error: prefix, even when the usage message itself is localized. Use CommandResult::error_msg instead to display the localized usage message without the error prefix.
| _ => CommandResult::error(t(MessageId::CmdTaskUsageGeneral), app.ui_locale), | |
| _ => CommandResult::error_msg(t(MessageId::CmdTaskUsageGeneral)), |
- Move ApiProvider import to cfg(test) block in core.rs, debug.rs - Move CostCurrency import to cfg(test) block in debug.rs - Remove dead family_label function, replace test call with family_label_locale
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let _ = writeln!(out, "Current session snapshot:"); | ||
| let _ = writeln!(out, "- Workspace: {}", app.workspace.display()); | ||
| let _ = writeln!(out, "- Mode: {}", app.mode.label()); | ||
| let _ = writeln!(out, "- Mode: {}", app.mode.label(app.ui_locale)); |
There was a problem hiding this comment.
SendMessage payload. Embedding app.mode.label(app.ui_locale) means a non-English locale will send a translated mode label into the model-facing context — the same issue that was already corrected in engine.rs and ui.rs by using Locale::En. Using the user's locale here would make the relay instruction inconsistent with StructuredState::capture() calls that always pass Locale::En.
| let _ = writeln!(out, "- Mode: {}", app.mode.label(app.ui_locale)); | |
| let _ = writeln!(out, "- Mode: {}", app.mode.label(Locale::En)); |
|
Greptile review identified two remaining wiring issues. Here are the specific fixes: Fix 1: FanoutCounts dead code in agent_card.rs (~line 321)- format!(
- "{done} done \u{00B7} {running} running \u{00B7} {failed} failed \u{00B7} {pending} pending"
- ),
+ self.app.tr(locale, MessageId::FanoutCounts),The FanoutCounts message ID was defined in localization.rs but never used — this wires it in. Fix 2: Relay instruction locale in commands/mod.rs (line 830)- let _ = writeln!(out, "- Mode: {}", app.mode.label(app.ui_locale));
+ let _ = writeln!(out, "- Mode: {}", app.mode.label(Locale::En));Relay instructions go to the model — they must use Locale::En for consistency with StructuredState::capture() and engine.rs. After these two fixes, the PR is ready to merge. 3438 tests still pass. |
2 participants
Rebases and supersedes PR #812.
Wires the Phase 1–4b MessageId translations into the actual UI layer across 47 files. Fixes 109 compile errors caused by the upstream rebase (E0004 non-exhaustive patterns, E0061 argument count mismatches).
Changes
Test status
3438 passed, 0 failures (2 pre-existing environment-specific tests do not apply in CI)
Greptile Summary
This PR wires the Phase 1–4b i18n
MessageIdtranslations into 46 UI files, renames the 1-argCommandResult::errortoerror_msgand introduces a locale-aware 2-argerror, and fixes 109 compile errors from the upstream rebase (non-exhaustive match arms, argument-count mismatches).AppMode::label,VimMode::label,ElevationOption::label/description, and all tool-card family labels now accept aLocaleparameter;HookContext, analytics events, andStructuredState::capture()calls are all correctly pinned toLocale::En.AppMode::labeldefault arm now propagatesLocale::Enin model-facing contexts; the qa_pty viewport assertion is broadened to accept both old title-case and new uppercase mode labels.Confidence Score: 4/5
Safe to merge after addressing two missed wiring spots: a model-facing relay instruction that embeds the user's locale instead of English, and a FanoutCounts message ID that was registered but never connected to the rendering code.
Two call sites were left partially wired. In build_relay_instruction the mode label is built with app.ui_locale rather than Locale::En, so the relay text sent to the model will contain a translated mode name once non-English locales are added — diverging from every other model-facing site in the same PR. In FanoutCard::render_lines the FanoutCounts message ID was declared and translated but the rendering code still uses a hardcoded format string, leaving fanout stats unlocalized and the new variant dead. Everything else — hook context, analytics, engine state capture, header, approval, context inspector, queue commands — is correctly wired.
crates/tui/src/commands/mod.rs (build_relay_instruction locale) and crates/tui/src/tui/widgets/agent_card.rs (FanoutCard stats row)
Important Files Changed
Comments Outside Diff (3)
crates/tui/src/commands/queue.rs, line 51-52 (link){n}placeholder in list contextCmdEditingQueuedDrafttranslates to"Editing queued message {n} (press Enter to re-queue/send)", but inlist_queuethe{n}placeholder is never replaced. When a user runs/queue listwith an active draft the output will literally contain"Editing queued message {n} (press Enter to re-queue/send):". The original string before this PR was a separate"Editing queued message:"without any placeholder, so these two sites need distinct message IDs — or the list site needs to call.replace("{n}", "")at minimum.crates/tui/src/commands/mod.rs, line 911-914 (link)#[allow(dead_code)]on a function with 109+ callerserror_msgis called at over 100 sites introduced in this very PR, so the compiler will not flag it as dead code. The annotation is a no-op right now but may mislead future readers into thinking the function is intended for eventual removal. Consider dropping the attribute, or — iferror_msgis genuinely transitional — leave a// TODO:comment explaining the migration plan.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
crates/tui/src/tui/widgets/agent_card.rs, line 318-328 (link)FanoutCountsmessage ID wired but never usedMessageId::FanoutCountswas added tolocalization.rswith the English translation"{done} done · {running} running · {failed} failed · {pending} pending", butFanoutCard::render_linesstill builds this string with a hardcoded format literal rather than callingtr(locale, MessageId::FanoutCounts). The new message ID is effectively dead code, and the fanout stats row will always display in English regardless of the active locale — the only part of the fanout card that was not actually wired in this PR.Reviews (2): Last reviewed commit: "fix: queue draft header {n} leak, hook/a..." | Re-trigger Greptile