fix(i18n): remove hardcoded strings; enforce mandatory pre-PR review - #232
Conversation
- speech recognition no longer defaults to ja-JP; callers must pass a locale - session handoff prompt and role labels switched from Japanese to English - slash command descriptions now use the existing slash_desc_* resources - localize snackbar/error messages (config saved, clone failed, MCP auth, provider disconnect) with translations in all eight locale files - diagnostics sheet shows real version/runtime state instead of placeholders - delete unused ThemePickerDialog with hardcoded strings - English sample data in drawer preview
- add the pre-pr-review skill: local gates, repo-reviewer verdict loop, review report recorded in the PR description - add the repo-reviewer subagent with an embedded map of this repository and its hard rules (i18n key parity, DI wiring, CI gates) - add the PR Review Gate workflow that fails PRs without an approved review report (Weblate bot PRs exempt) - AGENTS.md now requires the review before opening any PR
- show Starting… for the connection row while the runtime is connecting - document that SlashSuggestion.App.description is empty by design
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| pull_request: | ||
| # "edited" re-runs the gate when the PR description changes, so the review report can be | ||
| # added after the PR exists and the check turns green without a new push. | ||
| types: [opened, edited, synchronize, reopened] |
There was a problem hiding this comment.
[other · low]
synchronize や edited トリガーにより連続 push で同じジョブが重複実行されます。このジョブは PR ごとに結果が決まるゲートなので、concurrency によるグループ化と cancel-in-progress: true を設定し、直前の実行をキャンセルして冗長な実行を防ぐことを推奨します。
Suggestion:
| types: [opened, edited, synchronize, reopened] | |
| concurrency: | |
| group: pr-review-gate-${{ github.event.pull_request.number }} | |
| cancel-in-progress: true | |
| on: | |
| pull_request: | |
| # "edited" re-runs the gate when the PR description changes, so the review report can be | |
| # added after the PR exists and the check turns green without a new push. | |
| types: [opened, edited, synchronize, reopened] |
|
|
||
| jobs: | ||
| review-gate: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
[other · low]
このジョブには timeout-minutes が設定されていません。grep のみの軽量な処理とはいえ、ランナー上でステップがハングすると無期限に実行され、ランナーリソースを消費し続けます。timeout-minutes: 5 程度を明示することを推奨します。
Suggestion:
| runs-on: ubuntu-latest | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 5 |
| authNotRemovedMessage = context.getString(R.string.mcp_auth_not_removed), | ||
| authFailedTemplate = context.getString(R.string.mcp_auth_failed_status), |
There was a problem hiding this comment.
[maintainability · low]
viewModel(key = ...) は Key 単位でインスタンスをキャッシュするため、この Factory は ViewModel が初回生成されるときに一度だけ実行され、context.getString(...) で解決した文字列はそのまま ViewModel に保持され続けます。システム言語を変更して Activity が再生成されても ViewModel は破棄されず、以後のエラーメッセージが以前の言語のまま表示される可能性があります（特にナビゲーションで画面を離れて戻った場合もキャッシュが再利用されます）。実行時のロケール切替を考慮する場合は、文字列を ViewModel 生成時に固定するのではなく、表示時に再解決する（例: エラー表示側で resource ID を基に解決する）設計を検討してください。現状のままでも機能的には問題ないため、対応は任意です。
…n composition lintDebug flags StateFlowValueCalledInComposition for the diagnostics sheet; collect the selected runtime's state flow instead.
| on: | ||
| pull_request: | ||
| # "edited" re-runs the gate when the PR description changes, so the review report can be | ||
| # added after the PR exists and the check turns green without a new push. | ||
| types: [opened, edited, synchronize, reopened] |
There was a problem hiding this comment.
[performance · medium]
PRイベント(opened/edited/synchronize/reopened)で起動するこのジョブにはconcurrency設定がありません。連続プッシュや説明文編集のたびに同一のゲートチェックが並列実行され、リソースと実行時間を無駄に消費します。concurrencyグループとcancel-in-progress: trueを追加し、同じPRの古い実行をキャンセルすることを推奨します。
Suggestion:
| on: | |
| pull_request: | |
| # "edited" re-runs the gate when the PR description changes, so the review report can be | |
| # added after the PR exists and the check turns green without a new push. | |
| types: [opened, edited, synchronize, reopened] | |
| concurrency: | |
| group: pr-review-gate-${{ github.event.pull_request.number }} | |
| cancel-in-progress: true | |
| on: | |
| pull_request: | |
| # "edited" re-runs the gate when the PR description changes, so the review report can be | |
| # added after the PR exists and the check turns green without a new push. | |
| types: [opened, edited, synchronize, reopened] |
| jobs: | ||
| review-gate: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
[other · low]
ジョブにtimeout-minutesが設定されていません。万が一run:スクリプトがハングした場合、ジョブが無期限に実行されランナーリソースを占有します。単純なゲートチェックなので、数分のタイムアウトを設定して上限を明示することを推奨します。
Suggestion:
| jobs: | |
| review-gate: | |
| runs-on: ubuntu-latest | |
| jobs: | |
| review-gate: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 5 |
| data class App(val command: SlashCommand) : SlashSuggestion { | ||
| override val name: String = command.name | ||
| override val description: String = command.description | ||
| override val description: String = "" | ||
| override val descriptionRes: Int = command.descriptionRes | ||
| } |
There was a problem hiding this comment.
[maintainability · low]
App は description を常に空文字でオーバーライドしていますが、インターフェース SlashSuggestion は description: String を必須の抽象プロパティとして公開し続けています。descriptionRes の解決は UI 層でしか行われないため、description を直接参照する将来のコード（テスト・ログ・他機能など）はアプリコマンドで黙って空文字を表示してしまいます。description を String? にしデフォルトで null（App はオーバーライド不要）とし、Backend のみリテラル文字列を持つ設計にすると、このダミー空文字を排除でき、安全かつ簡潔になります。
Suggestion:
| data class App(val command: SlashCommand) : SlashSuggestion { | |
| override val name: String = command.name | |
| override val description: String = command.description | |
| override val description: String = "" | |
| override val descriptionRes: Int = command.descriptionRes | |
| } | |
| data class App(val command: SlashCommand) : SlashSuggestion { | |
| override val name: String = command.name | |
| override val descriptionRes: Int = command.descriptionRes | |
| } |
| authNotRemovedMessage: String = "Authentication was not removed", | ||
| authFailedTemplate: String = "Authentication failed: %1\$s", | ||
| ) : this(registry::targetFor, agent, authNotRemovedMessage, authFailedTemplate) |
There was a problem hiding this comment.
[maintainability · low]
プライマリコンストラクタとセカンダリコンストラクタの両方に同じデフォルトメッセージ文字列が重複して定義されています。今後デフォルト文言を変更する際に両方の修正が必要になり、片方だけ変更すると挙動が乖離するリスクがあります。companion object の const val に共通定義し、両コンストラクタで参照する形にすると重複を回避できます。
| if (showDiagnostics) { | ||
| val runtimeStateFlow = | ||
| remember(selectedRuntime) { | ||
| selectedRuntime?.state ?: MutableStateFlow(RuntimeState.Disconnected) |
There was a problem hiding this comment.
[maintainability · low]
selectedRuntime が null の場合に作成される MutableStateFlow(RuntimeState.Disconnected) は、以後どこからも更新されることがない実質的な定数フローです。flowOf(RuntimeState.Disconnected) を使うほうが意図が明確になり、このためだけに追加した MutableStateFlow import も不要になります。
Suggestion:
| selectedRuntime?.state ?: MutableStateFlow(RuntimeState.Disconnected) | |
| selectedRuntime?.state ?: flowOf(RuntimeState.Disconnected) |
| }, | ||
| runtimeStatus = | ||
| when (runtimeState) { | ||
| null, RuntimeState.Disconnected -> stringResource(R.string.disconnected_label) |
There was a problem hiding this comment.
[maintainability · low]
runtimeState は StateFlow<RuntimeState> に対して collectAsState() を呼んだ結果なので常に非 null です。したがって when の null 分岐は到達不能なデッドコードです。null, を削除して RuntimeState.Disconnected のみにしてください。
Suggestion:
| null, RuntimeState.Disconnected -> stringResource(R.string.disconnected_label) | |
| RuntimeState.Disconnected -> stringResource(R.string.disconnected_label) |
by-delegation from collectAsState() blocks smart casting in the when branches; bind State.value to a local val instead and drop the now-dead null branch.
| jobs: | ||
| review-gate: | ||
| runs-on: ubuntu-latest | ||
| # Weblate opens automated translation PRs; they cannot run the agent-side review skill. | ||
| if: github.event.pull_request.user.login != 'weblate' |
There was a problem hiding this comment.
[performance · low]
このワークフローは pull_request の synchronize（プッシュ毎）と edited（説明文の編集毎）で再実行されます。開発中にコミットを連続でプッシュすると、内容のほぼ同一なジョブが並列にキューされ、ランナーリソースを無駄に消費します。プルリクエスト単位の concurrency グループと cancel-in-progress: true を追加し、最新のジョブだけを実行することを推奨します。
Suggestion:
| jobs: | |
| review-gate: | |
| runs-on: ubuntu-latest | |
| # Weblate opens automated translation PRs; they cannot run the agent-side review skill. | |
| if: github.event.pull_request.user.login != 'weblate' | |
| concurrency: | |
| group: pr-review-gate-${{ github.event.pull_request.number }} | |
| cancel-in-progress: true | |
| jobs: | |
| review-gate: | |
| runs-on: ubuntu-latest | |
| # Weblate opens automated translation PRs; they cannot run the agent-side review skill. | |
| if: github.event.pull_request.user.login != 'weblate' |
1 participant
Summary
Two related changes:
1. Remove hardcoded strings and locale-specific behavior (
fix(i18n))ja-JP; callers must pass the user's locale tag, so dictation can never silently fall back to the wrong language. Both call sites already pass the device locale.slash_desc_*resources already existed in all eight locale files; the registry now carries@StringResids and the composer resolves them."0.3.0","connected","ready"); it now reportsBuildConfig.VERSION_NAMEand the real selected-runtime state.ThemePickerDialog(dead code full of hardcoded strings) and switched the drawer preview's sample data to English.2. Make pre-PR review mandatory (
ci)pre-pr-reviewskill (.opencode/skills/pre-pr-review/SKILL.md): run local gates, have therepo-reviewersubagent review the full diff againstorigin/main, fix blocking findings untilAPPROVE, and record the report in the PR description.repo-reviewersubagent (.opencode/agents/repo-reviewer.md): read-only reviewer whose prompt embeds this repository's architecture map and hard rules (i18n key parity across all eight locale files, Koin/ViewModelFactory DI wiring, CI gates), so it reviews like a maintainer who knows the repo.PR Review Gateworkflow: fails any PR (except Weblate's automated translation PRs) whose description lacks the<!-- pre-pr-review: approved -->marker, so the gate is enforced by CI, not just convention.AGENTS.mdnow requires the review before any PR is opened.Verification
./gradlew detekt spotlessCheckpasses locally.i18n-check.yml: all keys present in all eight locale files, no extras.Pre-PR review
Reviewer:
repo-reviewersubagent (invoked via the pre-pr-review skill workflow). Verdict below is for the i18n + review-mechanism changes; the final commit applies the reviewer's own two non-blocking suggestions (Connecting state on the diagnostics connection row; KDoc onSlashSuggestion.App.description).Incremental re-reviews (post-approval fixups)
Two fixup commits landed after the main review; each was re-reviewed by the same reviewer workflow:
8787e27(collect runtime state instead ofStateFlow.valuein composition, fixing thelintDebugStateFlowValueCalledInCompositionerror) — re-review verdict REQUEST_CHANGES:by collectAsState()delegation blocks smart casts in thewhenbranches (confirmed by CI compile errors on lines 1224–1226).09aae4e— fixes that blocker by bindingcollectAsState().valueto a local val and dropping the deadnullbranch. Re-review verdict APPROVE, no blockers. Full CI green on this HEAD.