Dedup CLI commands and fix server race + debug noise - #1
Conversation
- Extract ~600 lines of duplicated provider-command boilerplate into cmd/common.go (resolveGlobalOptions, typed flag helpers, runTranslation). Commands are now ~6 lines each and use RunE for proper exit codes. - Fix data race + redundant work in server runTranslation: translations are already applied incrementally under lock via applyResponse; the trailing unlocked ApplyTranslations raced with payload readers. - Remove per-request debug Printf in the OpenAI hot path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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 refactors the CLI commands by extracting shared flag resolution and translation pipeline logic into a new cmd/common.go file, and updates commands to return errors. It also avoids redundant translation applications in internal/server/server.go and removes a debug print statement in internal/translator/openai.go. Feedback highlights critical concurrency issues in internal/server/server.go—including data races on s.xcstrings, potential deadlocks from recursive read locking, and unhandled concurrent jobs—and suggests validating input and output file paths in cmd/common.go to improve error reporting.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- server.go: hold RLock across buildPayload (map reads no longer race with applyResponse), snapshot Job by value in handleProgress to avoid recursive RLock deadlock, keep RLock through MarshalXCStrings in handleExport, and take the write lock in handleTranslate to reject overlapping jobs and safely mutate/read shared state. Remove now-unused startJob. - cmd/common.go: validate input/output paths up front for clearer errors. - Add -race regression test for concurrent apply/read on ServerState. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Code review + optimization pass over the Go codebase. Two correctness/performance fixes and one large dedup.
1. Remove per-request debug print (hot path) —
internal/translator/openai.gotranslateOnceranfmt.Printf("OpenAI Translation Response status: %d\n", ...)on every string translated. Leftover debug noise in the busiest path. Removed.2. Fix data race + redundant work in the web server —
internal/server/server.gorunTranslationapplied each translation incrementally viaapplyResponse(unders.mu), then calledApplyTranslations(xc, responses)again at the end without holding the lock. That second pass re-did work, mutatedxc.StringswhilehandleProgress/handleStringsread the same map underRLock(a real data race during an in-flight job), and spammed errors to stdout. Removed it — incremental apply is now the single source of truth.3. Collapse ~600 lines of duplicated CLI boilerplate —
cmd/The four provider commands (
google,deepl,baidu,openai) each carried a near-identical ~150-line block (flag/viper resolution + load → translate → report → apply → save). Extracted into newcmd/common.go:resolveGlobalOptions+ typedstringFlag/boolFlag/intFlag/float64Flag/stringSliceFlaghelpersrunTranslation(...)for the shared pipelineEach command is now ~6 lines.
Behavior notes (intentional)
RunEand return errors, so load/save failures exit non-zero instead of the old silentreturn(exit 0). Provider-specific timeouts preserved (OpenAI 600s, others 300s).--verboseheader no longer echoes provider-only fields (model/glossary/formality/appID/baseURL/temperature/maxTokens). Core run config still printed; no flags removed.Testing
gofmt,go build ./...,go vet ./...all clean--helplists all commands correctlyNet: 7 files changed, +198 / −636.
🤖 Generated with Claude Code