Conversation
📝 WalkthroughWalkthroughAdds a GitHub Actions step "Run Locale check" and a small Node locale-audit script with package.json under .github/workflows/static-analysis/locale. Removes emails.magicSession.body and emails.magicSession.footer from many locale JSON files and adds a new "mock" key to en.json and de.json. Bumps utopia-php/locale in composer.json to 0.8.*. Configures Locale fallback in app/init/resources.php, app/controllers/api/projects.php, and Certificates worker. Adds a /v1/mock/tests/locale endpoint and an E2E test (testFallbackLocale) validating fallback behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
app/config/locale/translations/en.json (1)
277-279: Consider namespacing test-only translation keyAdding a test-only key at the root ("mock") works, but consider namespacing (e.g., "tests.mock") to avoid future collisions and keep production keys distinct from test artifacts.
Apply this diff here:
- "mock": "A mock translation for testing purposes." + "tests.mock": "A mock translation for testing purposes."Outside this file, update usages accordingly:
- app/controllers/mock.php: use "tests.mock"
- tests referencing the key: use "tests.mock"
.github/workflows/static-analysis/locale/package.json (1)
1-13: Mark package as private and avoid mismatched licensingThis is an internal CI helper under .github; mark it private and avoid publishing/licensing confusion by using UNLICENSED (or match the repo’s BSD-3). Also optionally declare the Node engine to align with the workflow (node:24).
Apply this diff:
{ "name": "static-analysis-locale", "version": "1.0.0", "type": "module", "main": "index.js", + "private": true, "scripts": { "test": "echo \"Error: no test specified\" && exit 1" }, "keywords": [], "author": "", - "license": "ISC", + "license": "UNLICENSED", + "engines": { + "node": ">=20" + }, "description": "" }.github/workflows/static-analysis/locale/index.js (6)
11-14: Make config overridable via env vars (for easy CI toggling)Allow CI to switch strict mode or fallback without code changes.
Apply this diff:
-const config = { - strict: false, - fallbackLocale: "en.json", -}; +const config = { + strict: process.env.LOCALE_STRICT === "true", + fallbackLocale: process.env.LOCALE_FALLBACK || "en.json", +};
49-56: Harden JSON parsing with file-aware errorsIf a locale file has invalid JSON, the current code throws without identifying the offending file. Add a try/catch to improve diagnostics and fail fast.
Apply this diff:
for (const file of files) { const filePath = join(translationsPath, file); const content = await readFile(filePath, "utf8"); - const translations = JSON.parse(content); + let translations; + try { + translations = JSON.parse(content); + } catch (e) { + console.error(`Invalid JSON in ${filePath}: ${e.message}`); + process.exit(1); + } // Add all keys from this file Object.keys(translations).forEach((key) => allKeys.add(key)); }
70-75: Harden JSON parsing for the checked locale(s)Mirror the above improvement for the per-locale read.
Apply this diff:
for (const localeToCheck of localesToCheck) { // Read locale const path = join(translationsPath, localeToCheck); const content = await readFile(path, "utf8"); - const translations = JSON.parse(content); + let translations; + try { + translations = JSON.parse(content); + } catch (e) { + console.error(`Invalid JSON in ${path}: ${e.message}`); + process.exit(1); + }
67-69: Rename counter to reflect actual meaning (missing keys, not locales)The variable tracks missing keys, not missing locales.
Apply this diff:
- let errorsCount = 0; - let missingLocaleCount = 0; + let errorsCount = 0; + let missingKeyCount = 0;
87-104: Clarify log messages and avoid “Fallback locale” wording in non-fallback casesMessages should be accurate in both strict and non-strict modes and reflect missing keys.
Apply this diff:
- if (missingKeys.length > 0) { - console.error( - `\nERROR: Fallback locale (${localeToCheck}) is missing ${missingKeys.length} key(s):`, - ); + if (missingKeys.length > 0) { + console.error( + `\nERROR: Locale (${localeToCheck}) is missing ${missingKeys.length} key(s):`, + ); missingKeys.sort().forEach((key) => { console.error(` - ${key}`); }); console.error( - `\nTo fix this issue, add the missing keys to ${translationsPath}/${localeToCheck}`, + `\nTo fix this issue, add the missing keys to ${translationsPath}/${localeToCheck}`, ); errorsCount++; - missingLocaleCount += missingKeys.length; + missingKeyCount += missingKeys.length; } else { console.log( - `\nSUCCESS: Fallback locale (${localeToCheck}) contains all ${allKeys.size} keys.`, + `\nSUCCESS: Locale (${localeToCheck}) contains all ${allKeys.size} keys.`, ); }
106-109: Fix summary grammar and countersUse “missing keys” and reflect the renamed counter.
Apply this diff:
- if (errorsCount > 0) { - console.log(`\n${missingLocaleCount} locales missing found across ${errorsCount} locales.`); - process.exit(1); - } + if (errorsCount > 0) { + console.log(`\n${missingKeyCount} missing keys found across ${errorsCount} locale(s).`); + process.exit(1); + }app/config/locale/translations/de.json (1)
248-248: Align signature with existing German pattern: use '{{project}}-Team'Other signatures in this locale use the hyphenated form (see lines 12, 16, 23, 30). For consistency, adjust this one as well.
- "emails.otpSession.signature": "{{project}} Team", + "emails.otpSession.signature": "{{project}}-Team",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (82)
.github/workflows/static-analysis.yml(1 hunks).github/workflows/static-analysis/locale/index.js(1 hunks).github/workflows/static-analysis/locale/package.json(1 hunks)app/config/locale/translations/af.json(0 hunks)app/config/locale/translations/ar-ma.json(0 hunks)app/config/locale/translations/ar.json(0 hunks)app/config/locale/translations/as.json(0 hunks)app/config/locale/translations/az.json(0 hunks)app/config/locale/translations/be.json(0 hunks)app/config/locale/translations/bg.json(0 hunks)app/config/locale/translations/bh.json(0 hunks)app/config/locale/translations/bn.json(0 hunks)app/config/locale/translations/bs.json(0 hunks)app/config/locale/translations/ca.json(0 hunks)app/config/locale/translations/cs.json(0 hunks)app/config/locale/translations/da.json(0 hunks)app/config/locale/translations/de.json(1 hunks)app/config/locale/translations/el.json(0 hunks)app/config/locale/translations/en.json(1 hunks)app/config/locale/translations/eo.json(0 hunks)app/config/locale/translations/es.json(0 hunks)app/config/locale/translations/fa.json(0 hunks)app/config/locale/translations/fi.json(0 hunks)app/config/locale/translations/fo.json(0 hunks)app/config/locale/translations/fr.json(0 hunks)app/config/locale/translations/ga.json(0 hunks)app/config/locale/translations/gu.json(0 hunks)app/config/locale/translations/he.json(0 hunks)app/config/locale/translations/hi.json(0 hunks)app/config/locale/translations/hr.json(0 hunks)app/config/locale/translations/hu.json(0 hunks)app/config/locale/translations/hy.json(0 hunks)app/config/locale/translations/id.json(0 hunks)app/config/locale/translations/is.json(0 hunks)app/config/locale/translations/it.json(0 hunks)app/config/locale/translations/ja.json(0 hunks)app/config/locale/translations/jv.json(0 hunks)app/config/locale/translations/km.json(0 hunks)app/config/locale/translations/kn.json(0 hunks)app/config/locale/translations/ko.json(0 hunks)app/config/locale/translations/la.json(0 hunks)app/config/locale/translations/lb.json(0 hunks)app/config/locale/translations/lt.json(0 hunks)app/config/locale/translations/lv.json(0 hunks)app/config/locale/translations/ml.json(0 hunks)app/config/locale/translations/mr.json(0 hunks)app/config/locale/translations/ms.json(0 hunks)app/config/locale/translations/nb.json(0 hunks)app/config/locale/translations/ne.json(0 hunks)app/config/locale/translations/nl.json(0 hunks)app/config/locale/translations/nn.json(0 hunks)app/config/locale/translations/or.json(0 hunks)app/config/locale/translations/pa.json(0 hunks)app/config/locale/translations/pl.json(0 hunks)app/config/locale/translations/pt-br.json(0 hunks)app/config/locale/translations/pt-pt.json(0 hunks)app/config/locale/translations/ro.json(0 hunks)app/config/locale/translations/ru.json(0 hunks)app/config/locale/translations/sa.json(0 hunks)app/config/locale/translations/sd.json(0 hunks)app/config/locale/translations/si.json(0 hunks)app/config/locale/translations/sk.json(0 hunks)app/config/locale/translations/sl.json(0 hunks)app/config/locale/translations/sn.json(0 hunks)app/config/locale/translations/sq.json(0 hunks)app/config/locale/translations/sv.json(0 hunks)app/config/locale/translations/ta.json(0 hunks)app/config/locale/translations/te.json(0 hunks)app/config/locale/translations/th.json(0 hunks)app/config/locale/translations/tl.json(0 hunks)app/config/locale/translations/tr.json(0 hunks)app/config/locale/translations/uk.json(0 hunks)app/config/locale/translations/ur.json(0 hunks)app/config/locale/translations/vi.json(0 hunks)app/config/locale/translations/zh-cn.json(0 hunks)app/config/locale/translations/zh-tw.json(0 hunks)app/controllers/api/projects.php(1 hunks)app/controllers/mock.php(2 hunks)app/init/resources.php(1 hunks)composer.json(1 hunks)src/Appwrite/Platform/Workers/Certificates.php(1 hunks)tests/e2e/Services/Locale/LocaleBase.php(1 hunks)
💤 Files with no reviewable changes (71)
- app/config/locale/translations/fr.json
- app/config/locale/translations/nl.json
- app/config/locale/translations/da.json
- app/config/locale/translations/hr.json
- app/config/locale/translations/ta.json
- app/config/locale/translations/sd.json
- app/config/locale/translations/lt.json
- app/config/locale/translations/ru.json
- app/config/locale/translations/pt-br.json
- app/config/locale/translations/zh-cn.json
- app/config/locale/translations/el.json
- app/config/locale/translations/hy.json
- app/config/locale/translations/az.json
- app/config/locale/translations/pa.json
- app/config/locale/translations/uk.json
- app/config/locale/translations/bh.json
- app/config/locale/translations/fo.json
- app/config/locale/translations/sa.json
- app/config/locale/translations/km.json
- app/config/locale/translations/ml.json
- app/config/locale/translations/eo.json
- app/config/locale/translations/bn.json
- app/config/locale/translations/zh-tw.json
- app/config/locale/translations/pl.json
- app/config/locale/translations/jv.json
- app/config/locale/translations/it.json
- app/config/locale/translations/vi.json
- app/config/locale/translations/tl.json
- app/config/locale/translations/si.json
- app/config/locale/translations/as.json
- app/config/locale/translations/cs.json
- app/config/locale/translations/nn.json
- app/config/locale/translations/ja.json
- app/config/locale/translations/af.json
- app/config/locale/translations/gu.json
- app/config/locale/translations/tr.json
- app/config/locale/translations/pt-pt.json
- app/config/locale/translations/ar-ma.json
- app/config/locale/translations/or.json
- app/config/locale/translations/ar.json
- app/config/locale/translations/ko.json
- app/config/locale/translations/fa.json
- app/config/locale/translations/lb.json
- app/config/locale/translations/fi.json
- app/config/locale/translations/ms.json
- app/config/locale/translations/bg.json
- app/config/locale/translations/he.json
- app/config/locale/translations/es.json
- app/config/locale/translations/is.json
- app/config/locale/translations/kn.json
- app/config/locale/translations/hi.json
- app/config/locale/translations/th.json
- app/config/locale/translations/bs.json
- app/config/locale/translations/lv.json
- app/config/locale/translations/hu.json
- app/config/locale/translations/ne.json
- app/config/locale/translations/sl.json
- app/config/locale/translations/nb.json
- app/config/locale/translations/ca.json
- app/config/locale/translations/sk.json
- app/config/locale/translations/te.json
- app/config/locale/translations/be.json
- app/config/locale/translations/mr.json
- app/config/locale/translations/sq.json
- app/config/locale/translations/ur.json
- app/config/locale/translations/id.json
- app/config/locale/translations/ga.json
- app/config/locale/translations/la.json
- app/config/locale/translations/ro.json
- app/config/locale/translations/sv.json
- app/config/locale/translations/sn.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/e2e/Services/Locale/LocaleBase.php (1)
src/Executor/Executor.php (1)
call(293-419)
app/init/resources.php (1)
src/Appwrite/Event/Audit.php (1)
setResource(32-37)
🪛 YAMLlint (1.37.1)
.github/workflows/static-analysis.yml
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 21-21: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (7)
composer.json (1)
63-63: composer.lock is up to date with utopia-php/locale 0.8.0The lock file already reflects the bump to version 0.8.*:
- composer.lock (lines 3947–3950):
"name": "utopia-php/locale", "version": "0.8.0",No mismatches found—composer.lock has been updated accordingly.
src/Appwrite/Platform/Workers/Certificates.php (1)
376-376: LGTM: Consistent fallback locale implementation.The addition of fallback locale configuration aligns with the broader fallback locale strategy implemented across the PR. This ensures email notifications use the environment locale as a fallback when translations are missing.
app/controllers/api/projects.php (1)
2269-2270: LGTM: Proper fallback locale configuration for email templates.The fallback locale is correctly set after constructing the Locale object, ensuring that when custom email templates are retrieved, missing translations will fall back to the environment locale (defaulting to 'en'). This is consistent with the fallback implementation pattern used throughout the PR.
app/init/resources.php (1)
73-77: LGTM: Enhanced locale resource with fallback configuration.The change from a simple arrow function to a multi-line closure properly configures the global Locale resource with fallback behavior. This ensures all injected locale instances throughout the application will have consistent fallback behavior to the environment locale.
app/controllers/mock.php (2)
16-16: LGTM: Proper import for Locale functionality.The import of the Locale class is correctly added to support the new mock endpoint functionality.
39-56: LGTM: Well-implemented mock endpoint for locale testing.The mock endpoint is properly implemented with:
- Correct parameter handling from both query params and headers
- Validation against available locale codes
- Proper use of Locale's setDefault method
- Clean response with the translated text
This endpoint effectively supports the E2E testing of fallback locale functionality.
tests/e2e/Services/Locale/LocaleBase.php (1)
273-324: LGTM: Comprehensive fallback locale testing.The test method thoroughly validates fallback locale behavior by testing:
- Default fallback when no locale header is provided
- Explicit English locale selection
- German locale selection with expected German translation
- Fallback to English for unsupported locales (
cs,non-existing)- Fallback to English for empty locale strings
The test cases cover all critical fallback scenarios and use appropriate assertions to verify both HTTP status codes and response content.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/static-analysis.yml (1)
17-17: Fix YAML lint: remove trailing whitespace on blank lineThere’s trailing whitespace on the otherwise blank Line 17. This breaks YAML linting.
Apply this minimal diff:
- +
🧹 Nitpick comments (3)
.github/workflows/static-analysis.yml (3)
18-21: Simplify Docker invocation: use working directory and quote $PWD
- Avoid subshell + cd by using Docker’s -w flag.
- Quote "$PWD" to be safe.
- docker run --rm -v $PWD:/app node:24-alpine sh -c \ - "cd /app/.github/workflows/static-analysis/locale && node index.js" + docker run --rm -v "$PWD":/app -w /app/.github/workflows/static-analysis/locale node:24-alpine node index.js
13-16: Rename step to reflect actual command (not CodeQL)The step runs composer checks, not CodeQL. Rename for clarity.
- - name: Run CodeQL + - name: Run Composer checksIf desired, also align the job’s display name (outside this changed range):
jobs: lint: name: Static analysis
20-21: Consider pinning container images/actions to immutable digestsFor supply-chain hardening, pin:
- node:24-alpine to a specific digest (e.g., node@sha256:…)
- actions/checkout@v4 to a commit SHA
Prevents unexpected changes from upstream tags.
Would you like me to propose a follow-up PR that resolves digests and pins them? I can generate the exact digests and update the workflow accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/static-analysis.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/static-analysis.yml
[error] 17-17: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: Benchmark
2 participants
What does this PR do?
Test Plan
Related PRs and Issues
x
Checklist