Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| translation = await manager.lookup_translation_with_fallback(content, **kwargs) | ||
| target = translation if translation is not None else content | ||
| locale = kwargs.get("_locale") or manager.get_locale() | ||
| return interpolate_message(target, kwargs, locale) |
There was a problem hiding this comment.
tx() propagates exceptions on network errors, violating its "never raises" contract
tx() is documented to "return the interpolated source on any failure (never raises or returns None)" — a contract also stated explicitly in the test file comments. However, when _translate_many raises a BaseException (e.g., a network error or timeout), _send_batch propagates it via fut.set_exception(exc). This exception then bubbles from tc.miss() → lookup_translation_with_fallback() → tx() with no interception.
The success: False path is correctly handled (miss() returns None, tx() falls back to content), but the exception path is not. A try/except is needed:
try:
translation = await manager.lookup_translation_with_fallback(content, **kwargs)
except Exception:
translation = None
target = translation if translation is not None else content
locale = kwargs.get("_locale") or manager.get_locale()
return interpolate_message(target, kwargs, locale)Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/gt-i18n/src/gt_i18n/translation_functions/_tx.py
Line: 34-37
Comment:
**`tx()` propagates exceptions on network errors, violating its "never raises" contract**
`tx()` is documented to "return the interpolated source on any failure (never raises or returns None)" — a contract also stated explicitly in the test file comments. However, when `_translate_many` raises a `BaseException` (e.g., a network error or timeout), `_send_batch` propagates it via `fut.set_exception(exc)`. This exception then bubbles from `tc.miss()` → `lookup_translation_with_fallback()` → `tx()` with no interception.
The `success: False` path is correctly handled (`miss()` returns `None`, `tx()` falls back to `content`), but the exception path is not. A try/except is needed:
```python
try:
translation = await manager.lookup_translation_with_fallback(content, **kwargs)
except Exception:
translation = None
target = translation if translation is not None else content
locale = kwargs.get("_locale") or manager.get_locale()
return interpolate_message(target, kwargs, locale)
```
How can I resolve this? If you propose a fix, please make it concise.| # Respect TTL — expired entries shouldn't leak stale data. | ||
| import time as _time | ||
|
|
||
| if entry.expires_at < _time.monotonic(): | ||
| return {} | ||
| return dict(entry.translations) |
There was a problem hiding this comment.
import time inside method body
import time as _time is deferred inside get_translations_sync rather than placed at the module level. Python caches imports so there's no performance issue, but the inline import is unconventional and inconsistent with how the same module (time) is imported at the top of _locales_cache.py. Add import time at the top of the file alongside the other stdlib imports and reference time.monotonic() directly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/gt-i18n/src/gt_i18n/i18n_manager/_i18n_manager.py
Line: 196-201
Comment:
**`import time` inside method body**
`import time as _time` is deferred inside `get_translations_sync` rather than placed at the module level. Python caches imports so there's no performance issue, but the inline import is unconventional and inconsistent with how the same module (`time`) is imported at the top of `_locales_cache.py`. Add `import time` at the top of the file alongside the other stdlib imports and reference `time.monotonic()` directly.
How can I resolve this? If you propose a fix, please make it concise.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!
| async def load_translations(self, locale: str | None = None) -> dict[str, str]: | ||
| """Load (or return cached) translations dict for the locale.""" | ||
| target = locale or self.get_locale() | ||
| return await self._translations.get_translations(target) | ||
| await self._locales_cache.miss(target) | ||
| entry = self._locales_cache._cache.get(target) | ||
| return dict(entry.translations) if entry else {} |
There was a problem hiding this comment.
_locales_cache._cache bypasses TTL eviction
Both load_translations (line 158) and get_translations_sync (line 193) read self._locales_cache._cache directly, bypassing LocalesCache.get() which enforces TTL by evicting expired entries. The race window is negligible in practice, but _cache is a protected attribute. LocalesCache could expose a get_translations_snapshot(locale) -> dict[str, str] helper to encapsulate the TTL logic in one place.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/gt-i18n/src/gt_i18n/i18n_manager/_i18n_manager.py
Line: 154-159
Comment:
**Direct access to `_locales_cache._cache` bypasses TTL eviction**
Both `load_translations` (line 158) and `get_translations_sync` (line 193) read `self._locales_cache._cache` directly, bypassing `LocalesCache.get()` which enforces TTL by evicting expired entries. The race window is negligible in practice, but `_cache` is a protected attribute. `LocalesCache` could expose a `get_translations_snapshot(locale) -> dict[str, str]` helper to encapsulate the TTL logic in one place.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Runtime translation port
Ports the runtime translation system from gt PR #1207 and PR #1217.
Changes
gt-i18n(minor) — replaces the monolithicTranslationsManagerwith a two-levelLocalesCache→TranslationsCachehierarchy (batched runtime fetches viaGT.translate_many, in-flight dedup, configurable concurrency cap, lifecycle hooks) and adds a public asynctx()for on-demand string translation. NewI18nManagermethods:lookup_translation,lookup_translation_with_fallback,load_translations,get_lookup_translation. Deprecatesget_translations,get_translation_resolver,resolve_translation_sync, andget_translation_loaderwithDeprecationWarning.hash_messagenow acceptsformat=and only appliesindex_vars()for ICU.gt-fastapi/gt-flask(minor) —initialize_gtaccepts new forwarded kwargs (lifecycle,batch_size,batch_interval_ms,max_concurrent_requests,translation_timeout_ms,cache_expiry_time); both packages now re-exporttxalongsidet.generaltranslation(patch) —hash_source/hash_templatepassensure_ascii=Falsetojson.dumpsso non-ASCII content hashes stay in parity with JSJSON.stringify. Found by bulk hash-parity tests.Greptile Summary
This PR ports the runtime translation system from the JS monorepo: replaces the monolithic
TranslationsManagerwith a two-levelLocalesCache→TranslationsCachehierarchy (batchedtranslate_manycalls, in-flight dedup, configurable concurrency cap, lifecycle hooks), adds the public asynctx()function, extendshash_messagewith aformat=parameter, fixes a cross-SDK hash divergence for non-ASCII content, and forwards new kwargs throughgt-fastapi/gt-flask.tx()documents that it "never raises or returns None" but is missing atry/exceptaroundlookup_translation_with_fallback— network errors from_send_batchpropagate as exceptions rather than falling back to the interpolated source.Confidence Score: 4/5
Safe to merge after fixing the tx() exception-fallback gap; all other changes are well-structured and well-tested.
One P1 issue: tx() propagates network exceptions instead of returning the interpolated source as documented and tested (success:False path works, exception path does not). The two-level cache hierarchy, batching logic, dedup, and deprecation wrappers are all correct. The non-ASCII hash fix and format= parameter on hash_message are clean and parity-tested.
packages/gt-i18n/src/gt_i18n/translation_functions/_tx.py — missing try/except for network error fallback
Important Files Changed
Sequence Diagram
sequenceDiagram participant User as Caller participant tx as tx() participant Mgr as I18nManager participant LC as LocalesCache participant TC as TranslationsCache participant GT as GT.translate_many User->>tx: await tx("Hello", _locale="es") tx->>Mgr: lookup_translation_with_fallback("Hello", ...) Mgr->>LC: get("es") [sync, TTL check] alt locale not cached LC->>LC: miss("es") → _fallback → load_translations("es") LC-->>Mgr: TranslationsCache(locale="es") end Mgr->>TC: miss({message, options}) alt hash in cache TC-->>Mgr: cached string else new miss TC->>TC: enqueue hash, schedule_batch / drain_queue TC->>GT: translate_many(sources, {target_locale: "es"}) GT-->>TC: {hash: {success, translation}} TC-->>Mgr: resolved string (or None on failure) end Mgr-->>tx: str | None tx-->>User: interpolated result (source fallback if None)Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "final touches" | Re-trigger Greptile