Conversation
Co-Authored-By: Virgil <virgil@lethean.io>
Rename ErrServiceNotInitialized → ErrServiceNotInitialised and update the error message string. Keeps a deprecated alias for the old name to avoid breaking the 21 downstream consumers. Co-Authored-By: Virgil <virgil@lethean.io>
NewCoreService() creates an i18n Core service factory so any binary can register i18n without depending on core/cli. Co-Authored-By: Virgil <virgil@lethean.io>
Co-Authored-By: Virgil <virgil@lethean.io>
ServiceOptions.ExtraFS accepts []FSSource — each pairs an fs.FS with a directory path. Translations load on top of the embedded defaults. Enables consuming packages to ship their own locale files. Co-Authored-By: Virgil <virgil@lethean.io>
Packages can now load translations in init(): i18n.LoadFS(locales.FS, ".") Co-Authored-By: Virgil <virgil@lethean.io>
i18n service now loads translations from three sources: 1. Embedded go-i18n base (grammar, verbs, nouns) 2. ExtraFS from ServiceOptions 3. Core.Locales() — collected from services implementing LocaleProvider Services just implement Locales() fs.FS and translations load automatically. Co-Authored-By: Virgil <virgil@lethean.io>
ExtraFS and Core.Locales() both go through AddLoader now for proper grammar handling. Package-level T() not resolving — needs debug. Co-Authored-By: Virgil <virgil@lethean.io>
Init().defaultOnce.Do() was replacing the service set by SetDefault(). Now Default() checks the atomic pointer first, and Init() skips if a service was already set. Fixes translations not appearing in CLI. Co-Authored-By: Virgil <virgil@lethean.io>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces Core framework integration for i18n via CoreService, adds runtime loader support for merging translations, renames error identifier to British spelling with backward compatibility alias, and updates dependency versions including new forge.lthn.ai/core/go requirement. Changes
Sequence DiagramssequenceDiagram
participant Core as Core Framework
participant NewCS as NewCoreService Factory
participant CS as CoreService
participant IS as i18n Service
participant Loader as FSLoader
participant MissingKeyCollector as Missing Key Collector
Core->>NewCS: NewCoreService(opts ServiceOptions)
NewCS->>IS: New() - Create i18n Service
NewCS->>Loader: Load translations from ExtraFS
Loader->>IS: Merge translation sources
NewCS->>IS: Apply language, mode, settings
NewCS->>CS: Return CoreService instance
NewCS->>Core: Register CoreService in runtime
Core->>CS: OnStartup()
CS->>IS: Check if in collect mode
alt Collect Mode Enabled
CS->>MissingKeyCollector: Subscribe to missing keys
IS->>MissingKeyCollector: Emit missing keys during translation
MissingKeyCollector->>CS: Store missing keys (thread-safe)
end
Core->>CS: MissingKeys()
CS-->>Core: Return collected missing keys
sequenceDiagram
participant App as Application
participant PkgFunc as Package AddLoader()
participant Svc as Service
participant Loader as Loader Instance
participant LangHandler as Language Handler
App->>PkgFunc: AddLoader(loader)
PkgFunc->>Svc: Get Default service
PkgFunc->>Svc: AddLoader(loader) method
loop For Each Language in Loader
Loader->>LangHandler: GetMessages(language)
LangHandler-->>Svc: Messages
Svc->>Svc: Merge messages (thread-safe)
alt Grammar Data Present
Loader->>LangHandler: GetGrammarData(language)
LangHandler-->>Svc: Grammar data
Svc->>Svc: SetGrammarData(grammar)
end
Svc->>Svc: Update available languages
end
Svc-->>PkgFunc: Return error status
PkgFunc-->>App: Complete AddLoader operation
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core_service.go`:
- Around line 67-69: The call to svc.SetLanguage(opts.Language) currently
ignores its error, allowing invalid ServiceOptions.Language values to silently
fall back to auto-detection; update the code path where SetLanguage is invoked
(the svc.SetLanguage(opts.Language) call) to capture and return (or propagate)
the error instead of discarding it so that invalid/unsupported language
overrides result in a visible error to the caller.
In `@service.go`:
- Around line 479-481: The call to SetGrammarData(lang, grammar) replaces the
entire cached grammar and should instead merge package-specific entries into the
existing grammar; retrieve the current grammar for lang (e.g., via
GetGrammarData(lang) or the cache accessor), create/ensure a non-nil target
object, merge grammar.Verbs, grammar.Nouns and grammar.Words into the existing
maps/slices (appending or copying keys/values as appropriate while avoiding
overwrites if desired), then call SetGrammarData(lang, mergedGrammar) (or update
the cache in place) so you augment rather than overwrite the base grammar loaded
by New; use the local variables lang and grammar and the SetGrammarData function
name to locate and apply the change.
- Around line 167-179: AddLoader currently mutates only the service returned by
Default() at call time, so loaders registered during package init are lost when
NewCoreService creates a fresh Service via New() and publishes it with
SetDefault(svc). Fix by introducing a persistent registry or by seeding new
services from the existing default: maintain a package-level slice/map of Loader
entries (e.g., loadersRegistry) that AddLoader(appends to) and ensure SetDefault
or NewCoreService copies/applies all entries from that registry into the new
Service (or have New/Service constructor consult the registry), referencing
AddLoader, Default, SetDefault, NewCoreService and New to locate where to add
the registry and apply the loaders.
- Around line 135-143: The current default init uses defaultOnce.Do with a
separate defaultService.Load() check and defaultService.Store(svc), which is a
TOCTOU race against concurrent SetDefault calls; fix by making the hand-off
atomic: after creating svc via New() inside the defaultOnce.Do, use
defaultService.CompareAndSwap(nil, svc) to store only if still nil (or wrap the
check/store with the same mutex used by SetDefault), and if CompareAndSwap
fails, close/cleanup svc if necessary to avoid leaking resources; update any
SetDefault/Init logic to rely on the atomic CompareAndSwap or the shared mutex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccedf9d4-1f3c-47de-9baa-431e5088b1c4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
core_service.gogo.modi18n.goi18n_test.goservice.go
| if opts.Language != "" { | ||
| _ = svc.SetLanguage(opts.Language) | ||
| } |
There was a problem hiding this comment.
Propagate invalid ServiceOptions.Language values.
Ignoring svc.SetLanguage(opts.Language) turns an invalid or unsupported override into a silent fallback to auto-detection, so the service can start in the wrong language with no signal to the caller.
Suggested fix
if opts.Language != "" {
- _ = svc.SetLanguage(opts.Language)
+ if err := svc.SetLanguage(opts.Language); err != nil {
+ return nil, err
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if opts.Language != "" { | |
| _ = svc.SetLanguage(opts.Language) | |
| } | |
| if opts.Language != "" { | |
| if err := svc.SetLanguage(opts.Language); err != nil { | |
| return nil, err | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core_service.go` around lines 67 - 69, The call to
svc.SetLanguage(opts.Language) currently ignores its error, allowing invalid
ServiceOptions.Language values to silently fall back to auto-detection; update
the code path where SetLanguage is invoked (the svc.SetLanguage(opts.Language)
call) to capture and return (or propagate) the error instead of discarding it so
that invalid/unsupported language overrides result in a visible error to the
caller.
| defaultOnce.Do(func() { | ||
| // If SetDefault was already called, don't overwrite | ||
| if defaultService.Load() != nil { | ||
| return | ||
| } | ||
| svc, err := New() | ||
| if err == nil { | ||
| defaultService.Store(svc) | ||
| loadRegisteredLocales(svc) | ||
| } |
There was a problem hiding this comment.
Make the Init/SetDefault hand-off atomic.
The check at Lines 137-138 and the store at Line 142 are a TOCTOU pair. A concurrent SetDefault(custom) can still land between them and then be overwritten by the service initialised here, which defeats the new “do not overwrite” behaviour. Use a CompareAndSwap(nil, svc) or a dedicated mutex around the check/store path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service.go` around lines 135 - 143, The current default init uses
defaultOnce.Do with a separate defaultService.Load() check and
defaultService.Store(svc), which is a TOCTOU race against concurrent SetDefault
calls; fix by making the hand-off atomic: after creating svc via New() inside
the defaultOnce.Do, use defaultService.CompareAndSwap(nil, svc) to store only if
still nil (or wrap the check/store with the same mutex used by SetDefault), and
if CompareAndSwap fails, close/cleanup svc if necessary to avoid leaking
resources; update any SetDefault/Init logic to rely on the atomic CompareAndSwap
or the shared mutex.
| // AddLoader loads translations from a Loader into the default service. | ||
| // Call this from init() in packages that ship their own locale files: | ||
| // | ||
| // //go:embed *.json | ||
| // var localeFS embed.FS | ||
| // func init() { i18n.AddLoader(i18n.NewFSLoader(localeFS, ".")) } | ||
| func AddLoader(loader Loader) { | ||
| svc := Default() | ||
| if svc == nil { | ||
| return | ||
| } | ||
| _ = svc.AddLoader(loader) | ||
| } |
There was a problem hiding this comment.
Package-level AddLoader registrations are lost after a later SetDefault.
This helper only patches whichever service Default() returns at that moment. NewCoreService in core_service.go builds a fresh Service with New() and then publishes it via SetDefault(svc), so any package locales loaded earlier from init() disappear at that point. The package-level API needs a persistent loader registry, or the replacement service needs to be seeded from the existing default before it is published.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service.go` around lines 167 - 179, AddLoader currently mutates only the
service returned by Default() at call time, so loaders registered during package
init are lost when NewCoreService creates a fresh Service via New() and
publishes it with SetDefault(svc). Fix by introducing a persistent registry or
by seeding new services from the existing default: maintain a package-level
slice/map of Loader entries (e.g., loadersRegistry) that AddLoader(appends to)
and ensure SetDefault or NewCoreService copies/applies all entries from that
registry into the new Service (or have New/Service constructor consult the
registry), referencing AddLoader, Default, SetDefault, NewCoreService and New to
locate where to add the registry and apply the loaders.
| // Merge grammar data into the global grammar store | ||
| if grammar != nil && (len(grammar.Verbs) > 0 || len(grammar.Nouns) > 0 || len(grammar.Words) > 0) { | ||
| SetGrammarData(lang, grammar) |
There was a problem hiding this comment.
Merge grammar data here instead of replacing it.
SetGrammarData in grammar.go overwrites the whole cache entry for lang. If an extra loader only contributes package-specific verbs, nouns, or words, this call wipes the base grammar loaded by New() instead of augmenting it, which breaks existing grammar-aware translations for that language.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service.go` around lines 479 - 481, The call to SetGrammarData(lang, grammar)
replaces the entire cached grammar and should instead merge package-specific
entries into the existing grammar; retrieve the current grammar for lang (e.g.,
via GetGrammarData(lang) or the cache accessor), create/ensure a non-nil target
object, merge grammar.Verbs, grammar.Nouns and grammar.Words into the existing
maps/slices (appending or copying keys/values as appropriate while avoiding
overwrites if desired), then call SetGrammarData(lang, mergedGrammar) (or update
the cache in place) so you augment rather than overwrite the base grammar loaded
by New; use the local variables lang and grammar and the SetGrammarData function
name to locate and apply the change.
1 participant
Forge → GitHub Sync
Commits: 9
Files changed: 6
Automated sync from Forge (forge.lthn.ai) to GitHub mirror.
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
Release Notes
New Features
Chores