Resolve (untested): 8221-improve-german-translation#8256
Resolve (untested): 8221-improve-german-translation#8256Schwachinformatiker wants to merge 12 commits into
Conversation
…d german. First shy commit.
Review: PR #8256 - DRAFT: 8221-improve-german-translationSummaryThis PR improves the German (de-DE) locale and fixes a hardcoded English string in the interface settings component. It addresses issue #8221. Changes Analyzed
Issues Found
VerdictThe code changes are correct and well-scoped. The i18n fix in Recommendation: fix needed Action items:
|
…re are some translations still missing.
| "errorInvalidType": "Ungültiger Typ '{{type}}' für Server '{{serverName}}'. Typ muss 'stdio', 'http' oder 'sse' sein", | ||
| "save": "Speichern" | ||
| "save": "Speichern", | ||
| "transportType": "Transport Methode" |
There was a problem hiding this comment.
Not sure if this is really a meaningful translation...
… into 8221-improve-german-translation
Schwachinformatiker
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Follow-up Review (new commits since Jun 3)Two new commits since my last review:
What ChangedThe PR now touches 17 files: 8 German locale files, matching English locale files for new keys, 8 component files converting hardcoded strings to Issues Found1. German typo in 2. Compound noun in 3. PR not marked as GitHub Draft: Cairo Build IssueRegarding the
Recommendation: improve neededThe translation work is high quality overall. Fix the two typos above and resolve the local testing before requesting merge. Once tested and the typos are corrected, this should be ready. |
|
@qnixsynapse I cannot make Edit: I found the solution. I need to install the devel version of it on fedora. Also for some more dependencies: |
Review: PR #8256 — Resolve (untested): 8221-improve-german-translationReviewing commit: OverviewThis PR extends and improves the German (de-DE) translation, adds missing English source keys to match, and converts several hardcoded English strings in component files to Critical Issues (build-breaking)1. Syntax error in The reasoning tooltip assignment uses const tooltipText =
reasoningValue === 'on'
? {t('common:reasoningForced')} // <-- SyntaxError
: reasoningValue === 'off'
? {t('common:reasoningDisabled')} // <-- SyntaxError
: {t('common:reasoningAuto')} // <-- SyntaxErrorFix: Remove the curly braces — these are JS ternary expressions, not JSX interpolations: const tooltipText =
reasoningValue === 'on'
? t('common:reasoningForced')
: reasoningValue === 'off'
? t('common:reasoningDisabled')
: t('common:reasoningAuto')2. Syntax error in Same issue — bare ) : (
{t('settings:localApiServer.serverStatusStopped')}
)This is inside a Fix: Remove the braces: ) : (
t('settings:localApiServer.serverStatusStopped')
)
Minor Issues3. 4. German translation nits (non-blocking):
5. Consistency: The English source file What Looks Good
TestingThe author explicitly notes this is untested due to build environment issues. Given the two syntax errors identified above, a build would indeed fail. After fixing those, manual verification of the German locale in-app would be needed. Recommendationfix needed — The two syntax errors in |
|
@Schwachinformatiker Thanks for coordinating! I compared both branches against
The real overlap is small — 22 keys in common.json — 22 overlapping keys (English / Mine / Theirs)
settings.json — 36 overlapping keys (English / Mine / Theirs)
Proposed split: you keep the code wiring + new That said — I'm fairly new to contributing here, so please take these as suggestions, not verdicts. I'd genuinely welcome your feedback and am happy to change any of my edits. And whatever merge order the maintainers prefer, I'm glad to rebase my side onto yours (or vice versa). Does this work for you? |
Follow-up Review (new commit since Jun 6)Reviewing commit: Critical Issues: ResolvedBoth syntax errors flagged in the Jun 6 review have been fixed:
Parallel PR CoordinationContributor Suggested path forward for maintainers: Merge #8256 first (it contains the code-level i18n wiring that the other PRs depend on), then have the other PRs rebase on top. Remaining Translation Nits (non-blocking)These are genuine translation errors that will affect German users:
What Looks Good
Recommendation: improve needed (minor)The critical build-breaking issues are resolved. What remains are several small German language nits (items 1–6 above). If the maintainers are comfortable addressing those in a follow-up PR, this is functionally merge-ready. If translation quality is held to a higher bar, a final fixup commit covering the items above would be helpful. |
|
@tobiasheinrichfaska Thank you for your support. I don't really care what version of translation we use (as long as the translation gets better :D), so we can use your version if you like. An alternative would be, that I remove my translations again, but I thinks this would be more confusing... I admit, this is also my first contribution to this project - my first contribution to open source in general. |
|
Thanks, that works great — and congrats on the first OSS contribution! 🎉 To make our PRs combine without conflicts regardless of merge order, how about this: I open a small PR against your branch that aligns the ~58 overlapping Mind if I open that PR into your After that, I'd suggest letting the small locale-only PRs (#8284/#8285/#8289) be reviewed first, then you rebase #8256 on top — it'll come through cleanly, and your unique parts (the code wiring, new |
|
Follow-up Review (new commits detected since last review) Reviewing commit: Status of Previous FeedbackThe Jun 9 review flagged 6 remaining nits after the critical syntax errors were fixed. Here is what "round 3" addressed:
Items 3, 4, and 5 are confirmed fixed. The tense fix for
This is also one of the cases New in Round 3The main commit also merged the latest Overall AssessmentAll critical (build-breaking) issues have been resolved since the Jun 6 review. The code-level i18n wiring is correct and clean. The remaining open item is the
Coordination NoteThe coordination discussion between @Schwachinformatiker and @tobiasheinrichfaska is constructive. @tobiasheinrichfaska has proposed opening a small PR against this branch to align the ~58 overlapping keys before merge. That approach is sound and would eliminate potential translation conflicts. Recommendation: improve needed (very minor)Four German strings still use |
|
@tobiasheinrichfaska Thank you for your support 🥳 Do you mean you make another PR that will contain the changes of your other three branches (#8284) that gets merged into my PR? Sounds good for me. You can open a PR against my branch :). To what PR should I rebase my PR after merging this new PR + review? Just for your information: I will be unresponsive for the next week or so. |
…anhq#8289 Aligns the 58 common/settings keys that both this branch and the locale PRs (janhq#8285 settings, janhq#8289 common) touch, so the efforts combine without merge conflicts. 54 keys adopt the locale-PR wording; the 4 keys we agreed read better here (addAssistant, threshold, topKDesc, runOnStartup) are left unchanged. Also picks up Model -> Modell on defaultModel*. Reviewed and approved by the contributor (native German speaker) before commit; AI-assisted. Signed-off-by: Tobias Heinrich <tobias.a.w.heinrich@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Concede this key to janhq#8256's wording (correct accusative) so the overlapping de-DE strings match across both PRs and merge cleanly. Reviewed and approved by the contributor (native German speaker) before commit; AI-assisted. Signed-off-by: Tobias Heinrich <tobias.a.w.heinrich@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Concede threshold -> "Ähnlichkeitsschwellenwert", topKDesc -> "Anzahl der zurückgegebenen Textpassagen." and runOnStartup -> "Autostart" to janhq#8256, so the overlapping de-DE strings match across both PRs and merge cleanly. Reviewed and approved by the contributor (native German speaker) before commit; AI-assisted. Signed-off-by: Tobias Heinrich <tobias.a.w.heinrich@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pointer for maintainers (cross-posted from #8285): @qnixsynapse asked about overlapping German i18n PRs. The plan we agreed here is in progress — my align PR Schwachinformatiker#1 makes #8256 and #8284/#8285/#8289 conflict-free on every overlapping |
fix(i18n): align overlapping de-DE strings with janhq#8284/janhq#8285/janhq#8289
|
The PR should now be merge ready (or do I need to rebase it? @tobiasheinrichfaska) |
|
🎉 Thanks for merging the align PR — and glad the heat finally broke! Short answer: no rebase needed to avoid conflicts. The align PR made the 58 overlapping On ordering, our agreed plan still holds and is now conflict-free either way:
Nothing's blocking on my end — all three of mine are mergeable. Really appreciate the collaboration! 🙏 |
2 participants
Describe Your Changes
This PR improves the translation in German as well as extends the translation.
It also replaces hard coded English text so it can be translated.
This PR is untested due to
make devdoes have problems on my computer (It complains about missing libraries but they are installed on my system).Fixes Issues
Self Checklist
Added relevant comments, esp in complex areasCreated issues for follow-up changes or refactoring needed