Feat/i18n #7
Feat/i18n #7
Conversation
📝 WalkthroughWalkthroughThis change introduces internationalization (i18n) support to the framework. A new I18n class manages locale loading and translation lookups. The Client initializes an I18n instance with configurable defaults. Context adds a translation helper method. TypeScript definitions and English locale strings are provided alongside a test script. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Client
participant I18n
participant FileSystem as File System
participant Context
App->>Client: Initialize with i18n & locales path
Client->>I18n: Create instance (defaultLocale, logger)
Client->>I18n: loadLocales(localesDir)
I18n->>FileSystem: Read locale files from directory
FileSystem-->>I18n: en-US.json content
I18n->>I18n: Cache locales in Map
App->>Context: Create context for interaction/message
Context->>Client: Access client instance
Context->>I18n: t(locale, key, args)
I18n->>I18n: Resolve translation with fallback
I18n->>I18n: Interpolate placeholders in value
I18n-->>Context: Translated string
Context-->>App: Context with translation helper
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/structures/Client.ts`:
- Around line 48-52: The constructor in Client currently calls the async method
i18n.loadLocales without awaiting it, causing locales to possibly be
unavailable; add an initialization mechanism (either an async init() method on
the Client class that callers must await, or a public readonly ready:
Promise<void> initialized from a private async initializeLocales() called in the
constructor) and move the loadLocales call into that async path (referencing
i18n.loadLocales, Client.constructor, and the new init/ready/initializeLocales
methods) so callers can await client.init() or await client.ready before using
client.i18n.t().
In `@src/structures/I18n.ts`:
- Around line 57-61: The interpolation loop in I18n.ts using new
RegExp(`{${k}}`, "g") can misbehave and risk ReDoS when keys contain regex
metacharacters; update the loop in the method containing this logic to either
perform a safe literal string replacement or escape the key before building the
RegExp (e.g., add an escapeRegex helper that replaces /[.*+?^${}()|[\]\\]/g with
an escaped form and use it when constructing the RegExp), then use that escaped
key in value.replace or switch to a non-regex string replace approach to
substitute String(v) for the placeholder.
- Line 21: The loadLocales function is failing because getFiles (in
src/utils/Files.ts) only looks for .ts/.js and therefore never returns .json
locale files; update the code by adding a JSON-specific file scanner (e.g., add
getJsonFiles(baseDir) in Files.ts that FastGlob.syncs "**/*.json" with the same
cwd/ignore/absolute options) and change loadLocales to call getJsonFiles(dir)
(or alternately pass a configurable glob pattern into getFiles) so loadLocales
actually discovers and loads .json locale files; reference getFiles,
getJsonFiles, and loadLocales when making the change.
- Around line 53-55: The method in src/structures/I18n.ts currently returns
non-string values (returns value when typeof value !== "string"), violating the
declared string return type; change the branch that handles non-strings to
coerce the value to a string (e.g., JSON.stringify(value)) and emit a warning
(console.warn or the existing logger) including the key and the original value,
so the function always returns a string; alternatively, if nested objects are
intentional, update the method signature to return string | Record<string, any>
and adjust callers accordingly.
In `@types/client.d.ts`:
- Around line 15-17: Add a short JSDoc comment to the I18nOptions interface
explaining the expected format for defaultLocale (e.g., BCP 47 language tags
like "en-US"); update the declaration for I18nOptions and the defaultLocale
property to include that comment so consumers immediately see the expected
locale format when viewing the type, referencing the I18nOptions interface and
the defaultLocale field.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
locales/en-US.jsonpackage.jsonsrc/structures/Client.tssrc/structures/Context.tssrc/structures/I18n.tstest-i18n.jstypes/client.d.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/structures/Client.ts (4)
types/client.d.ts (1)
FrameworkOptions(19-25)src/structures/I18n.ts (1)
I18n(5-65)test-i18n.js (1)
path(3-3)src/utils/Files.ts (1)
getProjectRoot(21-27)
src/structures/I18n.ts (3)
src/utils/logger/Logger.ts (2)
Logger(10-199)error(103-105)test-i18n.js (1)
logger(5-5)src/utils/Files.ts (1)
getFiles(4-20)
test-i18n.js (2)
src/utils/logger/Logger.ts (1)
Logger(10-199)src/structures/I18n.ts (1)
I18n(5-65)
🪛 ast-grep (0.40.5)
src/structures/I18n.ts
[warning] 58-58: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp({${k}}, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (9)
locales/en-US.json (1)
1-9: LGTM!The locale file structure is correct with proper JSON format, placeholder syntax (
{user}), and nested key support that aligns with thelodash.getusage inI18n.t.test-i18n.js (1)
8-17: Test will fail due togetFilesnot finding JSON files.This test script demonstrates the i18n API correctly, but it will not work until the issue in
I18n.loadLocalesis fixed (seesrc/structures/I18n.tsreview). ThegetFilesfunction only scans for.tsand.jsfiles, so no locales will be loaded.Once the locale loading is fixed, the test logic looks correct for demonstrating translations and fallback behavior.
package.json (1)
46-46: LGTM!Minor patch version bump for
@types/nodetype definitions.src/structures/Client.ts (1)
34-34: LGTM!The
i18nproperty is correctly typed asI18nand declared as readonly.src/structures/Context.ts (3)
43-51: LGTM!The
t()method correctly uses the interaction's locale when available, falling back to the default locale for message-based contexts. This provides proper i18n support at the context level.
53-70: LGTM!The
toJSONmethod now consistently includesauthorin both interaction and message branches, providing a unified structure for serialization.
25-31: Type guards using property checks are reliable.The property checks
"user" in this.dataand"author" in this.datacorrectly distinguish between the two types. Per discord.js documentation,ChatInputCommandInteractionhas a documenteduserproperty (the User who invoked the command), whileMessagehas a documentedauthorproperty (a User object). These properties are mutually exclusive between the two types, making the type guards both safe and correct.types/client.d.ts (2)
4-8: LGTM:localespath fits the existing paths shape.This cleanly extends
FrameworkPathsfor i18n assets.
19-25: LGTM:i18noption added toFrameworkOptions.Matches the new configuration surface cleanly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (this.options.paths?.locales) { | ||
| this.i18n.loadLocales( | ||
| path.join(getProjectRoot(), this.options.paths.locales) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing await: locales may not be loaded when the client is used.
loadLocales is an async function, but it's called without await. Since constructors cannot be async, any code using client.i18n.t() immediately after construction will find an empty locales Map.
🔧 Proposed fix: Add an initialization method or expose a ready promise
Option 1: Add an async init() method that must be awaited:
public async init(): Promise<void> {
if (this.options.paths?.locales) {
await this.i18n.loadLocales(
path.join(getProjectRoot(), this.options.paths.locales)
);
}
}Option 2: Expose a ready promise:
public readonly ready: Promise<void>;
constructor(opts: FrameworkOptions) {
// ... existing code ...
this.ready = this.initializeLocales();
}
private async initializeLocales(): Promise<void> {
if (this.options.paths?.locales) {
await this.i18n.loadLocales(
path.join(getProjectRoot(), this.options.paths.locales)
);
}
}Then callers can do: await client.ready;
🤖 Prompt for AI Agents
In `@src/structures/Client.ts` around lines 48 - 52, The constructor in Client
currently calls the async method i18n.loadLocales without awaiting it, causing
locales to possibly be unavailable; add an initialization mechanism (either an
async init() method on the Client class that callers must await, or a public
readonly ready: Promise<void> initialized from a private async
initializeLocales() called in the constructor) and move the loadLocales call
into that async path (referencing i18n.loadLocales, Client.constructor, and the
new init/ready/initializeLocales methods) so callers can await client.init() or
await client.ready before using client.i18n.t().
| return; | ||
| } | ||
|
|
||
| const files = getFiles(dir); |
There was a problem hiding this comment.
Critical: getFiles does not find JSON locale files.
The getFiles utility (in src/utils/Files.ts) only scans for **/*.ts and **/*.js files. Since locale files are .json, loadLocales will never find or load any locale data.
🐛 Proposed fix: Create a dedicated function for locale files or pass a glob pattern
Option 1: Create a new function in Files.ts for JSON files:
export function getJsonFiles(baseDir: string): string[] {
return FastGlob.sync(["**/*.json"], {
cwd: baseDir,
absolute: true,
ignore: ["node_modules/**", ".git/**"],
});
}Option 2: Use fs.readdirSync directly in loadLocales:
-const files = getFiles(dir);
+const files = require("fs")
+ .readdirSync(dir)
+ .filter((f: string) => f.endsWith(".json"))
+ .map((f: string) => path.join(dir, f));🤖 Prompt for AI Agents
In `@src/structures/I18n.ts` at line 21, The loadLocales function is failing
because getFiles (in src/utils/Files.ts) only looks for .ts/.js and therefore
never returns .json locale files; update the code by adding a JSON-specific file
scanner (e.g., add getJsonFiles(baseDir) in Files.ts that FastGlob.syncs
"**/*.json" with the same cwd/ignore/absolute options) and change loadLocales to
call getJsonFiles(dir) (or alternately pass a configurable glob pattern into
getFiles) so loadLocales actually discovers and loads .json locale files;
reference getFiles, getJsonFiles, and loadLocales when making the change.
| if (!value) return key; | ||
|
|
||
| if (typeof value !== "string") return value; |
There was a problem hiding this comment.
Type inconsistency: method returns non-string values despite string return type.
When value is not a string (e.g., the key points to a nested object), it's returned as-is. This contradicts the declared return type and could break callers expecting a string.
🔧 Proposed fix: Stringify or log warning for non-string values
-if (typeof value !== "string") return value;
+if (typeof value !== "string") {
+ this.logger.warn(`Translation key "${key}" is not a string, returning key.`);
+ return key;
+}Or update the return type to string | Record<string, any> if nested lookups are intentional.
📝 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 (!value) return key; | |
| if (typeof value !== "string") return value; | |
| if (!value) return key; | |
| if (typeof value !== "string") { | |
| this.logger.warn(`Translation key "${key}" is not a string, returning key.`); | |
| return key; | |
| } |
🤖 Prompt for AI Agents
In `@src/structures/I18n.ts` around lines 53 - 55, The method in
src/structures/I18n.ts currently returns non-string values (returns value when
typeof value !== "string"), violating the declared string return type; change
the branch that handles non-strings to coerce the value to a string (e.g.,
JSON.stringify(value)) and emit a warning (console.warn or the existing logger)
including the key and the original value, so the function always returns a
string; alternatively, if nested objects are intentional, update the method
signature to return string | Record<string, any> and adjust callers accordingly.
| if (args) { | ||
| for (const [k, v] of Object.entries(args)) { | ||
| value = value.replace(new RegExp(`{${k}}`, "g"), String(v)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Escape regex special characters in interpolation keys.
If an args key contains regex metacharacters (e.g., $amount, user.name), the RegExp will either break or produce unexpected matches. Additionally, the static analysis tool flagged a potential ReDoS risk.
🔧 Proposed fix: Use string replacement or escape the key
if (args) {
for (const [k, v] of Object.entries(args)) {
- value = value.replace(new RegExp(`{${k}}`, "g"), String(v));
+ value = value.split(`{${k}}`).join(String(v));
}
}Alternatively, escape special regex characters:
const escapeRegex = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
value = value.replace(new RegExp(`\\{${escapeRegex(k)}\\}`, "g"), String(v));🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 58-58: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp({${k}}, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@src/structures/I18n.ts` around lines 57 - 61, The interpolation loop in
I18n.ts using new RegExp(`{${k}}`, "g") can misbehave and risk ReDoS when keys
contain regex metacharacters; update the loop in the method containing this
logic to either perform a safe literal string replacement or escape the key
before building the RegExp (e.g., add an escapeRegex helper that replaces
/[.*+?^${}()|[\]\\]/g with an escaped form and use it when constructing the
RegExp), then use that escaped key in value.replace or switch to a non-regex
string replace approach to substitute String(v) for the placeholder.
| export interface I18nOptions { | ||
| defaultLocale?: string; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: document expected locale format for defaultLocale.
A brief JSDoc hint (e.g., “en-US”) would reduce ambiguity for consumers.
✨ Suggested doc tweak
export interface I18nOptions {
+ /** Default locale identifier (e.g., "en-US"). */
defaultLocale?: string;
}🤖 Prompt for AI Agents
In `@types/client.d.ts` around lines 15 - 17, Add a short JSDoc comment to the
I18nOptions interface explaining the expected format for defaultLocale (e.g.,
BCP 47 language tags like "en-US"); update the declaration for I18nOptions and
the defaultLocale property to include that comment so consumers immediately see
the expected locale format when viewing the type, referencing the I18nOptions
interface and the defaultLocale field.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.