feat: i18n support for backend #1647
Conversation
WalkthroughThe pull request introduces a comprehensive localization strategy across the server-side codebase, focusing on implementing a dynamic message handling system. The changes involve introducing new services like Changes
Sequence DiagramsequenceDiagram
participant Client
participant LanguageMiddleware
participant TranslationService
participant StringService
participant Controller
Client->>LanguageMiddleware: Send request with Accept-Language header
LanguageMiddleware->>TranslationService: Set language
LanguageMiddleware->>StringService: Set language
StringService-->>LanguageMiddleware: Language set
TranslationService-->>LanguageMiddleware: Language set
LanguageMiddleware->>Controller: Forward request
Controller->>StringService: Request localized message
StringService->>TranslationService: Retrieve translation
TranslationService-->>StringService: Return translated message
StringService-->>Controller: Return localized message
Controller->>Client: Respond with localized message
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (6)
Server/middleware/isAllowed.js (1)
Line range hint
15-19: There's vomit on his sweater: Security alert! 🍝Exposing different languages in auth errors could lead to language enumeration attacks. Consider using generic messages for auth failures.
- Use generic messages for auth failures
- Log detailed errors server-side only
- Apply the same error creation helper as suggested for verifyOwnership
+ const createAuthError = (status, language) => { + // Use a generic message for all auth failures + const error = new Error(errorMessages.AUTHENTICATION_FAILED(language || 'en')); + error.status = status; + error.service = SERVICE_NAME; + return error; + }; - const error = new Error(errorMessages.NO_AUTH_TOKEN(req.language)); - error.status = 401; - error.service = SERVICE_NAME; + const error = createAuthError(401, req.language); + logger.debug({ + actualError: 'NO_AUTH_TOKEN', + language: req.language + });Also applies to: 24-28, 44-48
Server/middleware/verifyJWT.js (2)
Line range hint
81-84: Inconsistent i18n implementation in error messagesSome error messages are internationalized while others (EXPIRED_REFRESH_TOKEN, INVALID_REFRESH_TOKEN) are still static.
const errorMessage = refreshErr.name === "TokenExpiredError" - ? errorMessages.EXPIRED_REFRESH_TOKEN - : errorMessages.INVALID_REFRESH_TOKEN; + ? errorMessages.EXPIRED_REFRESH_TOKEN(req.language) + : errorMessages.INVALID_REFRESH_TOKEN(req.language);
Line range hint
91-93: Missing i18n support for REQUEST_NEW_ACCESS_TOKEN messageThe REQUEST_NEW_ACCESS_TOKEN message is not internationalized.
res.status(403).json({ success: false, - msg: errorMessages.REQUEST_NEW_ACCESS_TOKEN, + msg: errorMessages.REQUEST_NEW_ACCESS_TOKEN(req.language), });Server/controllers/inviteController.js (1)
Line range hint
64-64: Hardcoded email subject needs i18n supportThe email subject "Welcome to Uptime Monitor" should be internationalized.
- "Welcome to Uptime Monitor" + successMessages.WELCOME_EMAIL_SUBJECT(req.language)Server/tests/db/inviteModule.test.js (1)
Add test cases for non-English languages in inviteModule.test.js
The codebase has a complete translation system through
translationService.js, but the test suite only verifies English language responses. Add test cases with non-English languages to ensure error messages are correctly translated, especially for:
- Error message translation verification
- Fallback behavior when translations are missing
- Different accept-language header scenarios
🔗 Analysis chain
Line range hint
68-96: Add test case for non-English language.The test suite needs coverage for non-English languages to ensure i18n works correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any test cases for non-English languages rg -l "language: '(?!en)[a-z]{2}'" "Server/tests"Length of output: 296
Script:
#!/bin/bash # Search for mockLanguage definition and usage rg "mockLanguage\s*=|const\s+mockLanguage" Server/tests/ # Search for language-related test files fd -t f "language" Server/tests/ # Look for error messages implementation rg "AUTH_INVITE_NOT_FOUND" Server/Length of output: 1174
Script:
#!/bin/bash # Look for translation files and implementations rg "getTranslatedMessage" Server/ # Search for language/translation configuration files fd -e json -e js -t f "translation|language|locale" Server/ # Check for any non-English strings in the codebase rg "'[a-z]{2}'" Server/Length of output: 13892
Server/middleware/verifySuperAdmin.js (1)
Line range hint
51-52: Add language parameter to unauthorized error message.The unauthorized error message is not using the language parameter.
-return res.status(401).json({ success: false, msg: errorMessages.UNAUTHORIZED }); +return res.status(401).json({ success: false, msg: errorMessages.UNAUTHORIZED(req.language) });
🧹 Nitpick comments (15)
Server/tests/db/userModule.test.js (1)
30-31: Yo dawg, let's make this language thing more flexible!The hardcoded 'en' value limits our test coverage. Consider parameterizing this to test different language scenarios.
-const mockLanguage = 'en'; +const TEST_LANGUAGES = ['en', 'fr', 'es']; +const mockLanguage = TEST_LANGUAGES[0]; // Default to EnglishServer/service/translationService.js (1)
100-113: Validate language list before using it.This loop is correct in principle, but you might consider verifying that
response.data.result.languagesis defined and not empty. One slip, and you’ll be cleaning sauce off your sweater. A quick check can avoid any uncaught TypeError.Server/utils/messages.js (1)
45-46: Guard dynamic placeholders carefully.Replacing
'{monitorId}'with user input is swell, but ensure you sanitize or validate these placeholders in other contexts if they come from outer space. We don’t want our code slipping on spilled spaghetti if the placeholder is maliciously crafted.Server/middleware/languageMiddleware.js (1)
1-4: Enhance language validation.Keep calm, no need to lose your lunch: slicing to two characters may be too restrictive in certain locales (e.g., “zh-HK”). Consider implementing a more flexible approach if you anticipate regional codes. Otherwise, this short approach is fine for simpler use-cases.
- req.language = acceptLanguage.split(',')[0].slice(0, 2).toLowerCase(); + const preferredLang = acceptLanguage.split(',')[0].toLowerCase(); + // Optionally validate or parse the full locale if needed + req.language = preferredLang.length > 2 ? preferredLang.substring(0,2) : preferredLang;Server/controllers/inviteController.js (1)
Line range hint
59-59: Consider internationalizing email template selectionThe template name "employeeActivationTemplate" could be language-specific.
- "employeeActivationTemplate", + `employeeActivationTemplate_${req.language}`,Server/tests/db/inviteModule.test.js (1)
17-17: Consider using a constant for language code.Yo! The hardcoded language code 'en' should be moved to a shared constants file to maintain consistency across tests.
- const mockLanguage = 'en'; + const { DEFAULT_LANGUAGE } = require('../../constants'); + const mockLanguage = DEFAULT_LANGUAGE;Server/controllers/checkController.js (1)
39-41: Consider extracting response builder.Yo! These success response blocks are repeated across multiple methods. Let's DRY it up!
+ _buildSuccessResponse(messageType, data = null) { + return { + msg: successMessages[messageType](this.req.language), + ...(data && { data }), + }; + } + createCheck = async (req, res, next) => { // ... existing code ... - return res.success({ - msg: successMessages.CHECK_CREATE(req.language), - data: check, - }); + return res.success(this._buildSuccessResponse('CHECK_CREATE', check)); };Also applies to: 60-62, 80-82, 100-102, 120-122, 147-149
Server/tests/controllers/checkController.test.js (1)
17-19: Mom's spaghetti moment - we need more test cases!The tests only cover English language ('en'). We should add test cases for different languages to ensure proper i18n functionality.
Consider adding tests like:
it("should return localized success message for different languages", async function () { const languages = ['fr', 'es', 'de']; for (const lang of languages) { req.language = lang; await createCheck(req, res, next); expect(res.json.firstCall.args[0].msg).to.equal( successMessages.CHECK_CREATE(lang) ); } });Server/tests/controllers/maintenanceWindowController.test.js (1)
19-19: Knees weak, tests are heavy - need more language coverage!Multiple instances of hardcoded 'en' language. We should parameterize these tests to cover various language scenarios.
Consider using a test helper function:
function runTestWithMultipleLanguages(testFn) { const languages = ['en', 'fr', 'es', 'de']; languages.forEach(lang => { it(`should work with ${lang} language`, async function() { req.language = lang; await testFn(req, res, next); }); }); }Also applies to: 125-125, 182-182, 364-364
Server/service/networkService.js (1)
Line range hint
1-1: Yo, we need to document these i18n changes!The NetworkService class needs JSDoc updates to document the new language support.
Add language parameter documentation:
/** * @typedef {Object} JobData * @property {string} url - The URL to process * @property {string} _id - The monitor ID * @property {string} [language='en'] - The language code for i18n messages */Server/controllers/monitorController.js (1)
Line range hint
559-630: Refactor duplicated lookup stages in aggregation pipelineThe MongoDB aggregation pipeline contains three nearly identical lookup stages for different check types. This duplication makes the code harder to maintain and more prone to errors.
Consider extracting the lookup logic into a helper function:
const createCheckLookup = (checkType) => ({ $lookup: { from: `${checkType}checks`, let: { monitorId: "$_id" }, pipeline: [ { $match: { $expr: { $eq: ["$monitorId", "$$monitorId"] } } }, { $sort: { createdAt: -1 } }, ...(limit ? [{ $limit: limit }] : []), ], as: `${checkType}checks` } }); // Usage in pipeline: ...(limit ? [ createCheckLookup('standard'), createCheckLookup('pagespeed'), createCheckLookup('hardware') ] : []),Server/tests/controllers/authController.test.js (2)
21-21: Consider using a more descriptive mock language constant name.The constant
mockLanguagecould be more descriptive to indicate its test purpose.-const mockLanguage = 'en'; +const TEST_MOCK_LANGUAGE = 'en';
246-246: Ensure consistent language property initialization.The
languageproperty is added in multiple test setups. Consider extracting this to a shared test setup helper.// Create a helper function function createTestRequest(overrides = {}) { return { language: 'en', body: {}, params: {}, query: {}, ...overrides }; }Also applies to: 384-384, 452-452, 590-590, 705-705, 822-822, 940-940, 1058-1058
Server/index.js (2)
204-206: Yo, these empty lines are making me lose myself! 🎵While the initialization call is solid, we've got some extra empty lines that could use a cleanup.
ServiceRegistry.register(TranslationService.SERVICE_NAME, translationService); - - await translationService.initialize();
Line range hint
77-278: Yo, let's talk about the bigger picture of this i18n setup! 🎨The i18n implementation follows a clean service-based architecture, but consider these architectural points:
- Configuration Management: Consider moving POEditor configuration to the SettingsService for centralized management
- Error Handling: Ensure TranslationService gracefully handles POEditor API failures
- Caching Strategy: Consider implementing translation caching to reduce API calls
Would you like me to provide example code for any of these suggestions?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
Server/controllers/authController.js(18 hunks)Server/controllers/checkController.js(6 hunks)Server/controllers/inviteController.js(2 hunks)Server/controllers/maintenanceWindowController.js(6 hunks)Server/controllers/monitorController.js(13 hunks)Server/controllers/queueController.js(4 hunks)Server/controllers/settingsController.js(2 hunks)Server/controllers/statusPageController.js(2 hunks)Server/db/mongo/modules/inviteModule.js(2 hunks)Server/db/mongo/modules/monitorModule.js(4 hunks)Server/db/mongo/modules/recoveryModule.js(2 hunks)Server/db/mongo/modules/statusPageModule.js(1 hunks)Server/db/mongo/modules/userModule.js(2 hunks)Server/index.js(4 hunks)Server/middleware/isAllowed.js(3 hunks)Server/middleware/languageMiddleware.js(1 hunks)Server/middleware/verifyJWT.js(3 hunks)Server/middleware/verifyOwnership.js(2 hunks)Server/middleware/verifySuperAdmin.js(1 hunks)Server/service/jobQueue.js(2 hunks)Server/service/networkService.js(4 hunks)Server/service/translationService.js(1 hunks)Server/tests/controllers/authController.test.js(38 hunks)Server/tests/controllers/checkController.test.js(21 hunks)Server/tests/controllers/maintenanceWindowController.test.js(18 hunks)Server/tests/controllers/monitorController.test.js(44 hunks)Server/tests/db/inviteModule.test.js(4 hunks)Server/tests/db/monitorModule.test.js(78 hunks)Server/tests/db/recoveryModule.test.js(6 hunks)Server/tests/db/statusPageModule.test.js(3 hunks)Server/tests/db/userModule.test.js(17 hunks)Server/tests/services/networkService.test.js(27 hunks)Server/tests/utils/messages.test.js(1 hunks)Server/utils/formattedKey.js(1 hunks)Server/utils/messages.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Server/tests/services/networkService.test.js
- Server/tests/db/monitorModule.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
Server/tests/controllers/authController.test.js
[error] 350-350: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (11)
Server/tests/db/userModule.test.js (1)
32-32: There's vomit on his sweater already: We're missing critical i18n test coverage!The test suite needs additional coverage for:
- Language middleware integration
- POEditor API integration
- Default language fallback scenarios
Would you like me to help create these test cases?
Server/service/translationService.js (2)
18-35: Ensure graceful fallback in the initialize method.Knees weak, code sweaty—this fallback approach is almost as comforting as a warm bowl of mom’s spaghetti. However, consider handling potential partial loading cases, so if some files fail but others load, we still fall back to partial translations before pulling from POEditor.
184-198: Stay consistent with fallback logic.The fallback to English here matches the fallback logic in other parts of the service. This is as reliable as your favourite pasta sauce. Good work ensuring consistent fallback behaviour across the translation workflow.
Server/utils/messages.js (1)
4-8: Keep an eye on runtime dependencies.The translation service is retrieved from the registry, which is great for modular design. Much like ensuring you have enough noodles for your sauce, confirm the service is always registered before this code runs to avoid potential undefined reference errors.
Server/controllers/statusPageController.js (2)
26-26: Yo dawg, we need to validate that language param! 🍝The
req.languageis being used without validation. If it's undefined, the success messages might break.Add a default fallback:
- msg: successMessages.STATUS_PAGE_CREATE(req.language), + msg: successMessages.STATUS_PAGE_CREATE(req.language || 'en'),Also applies to: 45-45
43-43:⚠️ Potential issueMom's spaghetti moment: DB call needs error handling! 🍝
The language parameter is passed to DB but there's no error handling if the translation fails.
Add try-catch for translation errors:
- const statusPage = await this.db.getStatusPageByUrl(url, req.language); + try { + const statusPage = await this.db.getStatusPageByUrl(url, req.language); + } catch (translationError) { + logger.warn({ + message: `Translation failed, falling back to default language`, + error: translationError, + language: req.language + }); + const statusPage = await this.db.getStatusPageByUrl(url, 'en'); + }Server/controllers/monitorController.js (1)
46-46: Well-implemented i18n support for success messages!The consistent addition of
req.languageparameter to all success messages demonstrates a thorough implementation of internationalization support.Also applies to: 67-67, 79-79, 108-108, 136-136, 190-190, 234-234, 312-312, 404-404, 441-442, 472-472, 491-491
Server/controllers/authController.js (1)
156-156: Well-implemented i18n support for error messages!The consistent addition of
req.languageparameter to all error messages demonstrates a thorough implementation of internationalization support.Also applies to: 209-209, 279-279, 303-303
Server/tests/controllers/monitorController.test.js (1)
31-31: Maintain consistency with language property initialization.Similar to authController.test.js, consider using a shared test setup helper for request objects with language property.
Also applies to: 76-76, 124-124, 186-186, 251-251, 322-322, 384-384, 452-452, 590-590, 705-705, 822-822, 940-940, 1058-1058
Server/index.js (2)
77-78: Yo, these imports are straight fire! 🔥The imports are properly organized and follow the existing pattern in the file.
202-202: This service registration is cleaner than mom's spaghetti! 👌The TranslationService is properly registered with the ServiceRegistry, maintaining consistency with other services.
Server/tests/db/userModule.test.js
Outdated
| describe("getUserByEmail", function () { | ||
| it("throw an error if a user is not found", async function () { | ||
| userFindOneStub.returns({ | ||
| select: sinon.stub().resolves(null), | ||
| }); | ||
| try { | ||
| await getUserByEmail(mockUser.email); | ||
| } catch (error) { | ||
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND); | ||
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(mockLanguage)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mom's spaghetti moment: We need more language test cases!
The test only verifies the English error message. Let's add test cases for other languages to ensure our i18n implementation is solid.
describe("getUserByEmail", function () {
+ TEST_LANGUAGES.forEach(language => {
+ it(`throw an error if a user is not found (${language})`, async function () {
userFindOneStub.returns({
select: sinon.stub().resolves(null),
});
try {
await getUserByEmail(mockUser.email);
} catch (error) {
- expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(mockLanguage));
+ expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(language));
}
});
+ });
});📝 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.
| describe("getUserByEmail", function () { | |
| it("throw an error if a user is not found", async function () { | |
| userFindOneStub.returns({ | |
| select: sinon.stub().resolves(null), | |
| }); | |
| try { | |
| await getUserByEmail(mockUser.email); | |
| } catch (error) { | |
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND); | |
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(mockLanguage)); | |
| } | |
| }); | |
| describe("getUserByEmail", function () { | |
| TEST_LANGUAGES.forEach(language => { | |
| it(`throw an error if a user is not found (${language})`, async function () { | |
| userFindOneStub.returns({ | |
| select: sinon.stub().resolves(null), | |
| }); | |
| try { | |
| await getUserByEmail(mockUser.email); | |
| } catch (error) { | |
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(language)); | |
| } | |
| }); | |
| }); | |
| }); |
Server/tests/db/userModule.test.js
Outdated
| it("should throw an error if a user is not found", async function () { | ||
| try { | ||
| await deleteUser("testId"); | ||
| } catch (error) { | ||
| expect(error).to.exist; | ||
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND); | ||
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(mockLanguage)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Knees weak, arms are heavy: Same i18n gap in deleteUser tests!
Similar to getUserByEmail, we should test error messages in multiple languages.
-it("should throw an error if a user is not found", async function () {
+TEST_LANGUAGES.forEach(language => {
+ it(`should throw an error if a user is not found (${language})`, async function () {
try {
await deleteUser("testId");
} catch (error) {
expect(error).to.exist;
- expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(mockLanguage));
+ expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(language));
}
});
+});📝 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.
| it("should throw an error if a user is not found", async function () { | |
| try { | |
| await deleteUser("testId"); | |
| } catch (error) { | |
| expect(error).to.exist; | |
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND); | |
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(mockLanguage)); | |
| } | |
| }); | |
| TEST_LANGUAGES.forEach(language => { | |
| it(`should throw an error if a user is not found (${language})`, async function () { | |
| try { | |
| await deleteUser("testId"); | |
| } catch (error) { | |
| expect(error).to.exist; | |
| expect(error.message).to.equal(errorMessages.DB_USER_NOT_FOUND(language)); | |
| } | |
| }); | |
| }); |
Server/service/translationService.js
Outdated
| // Her dil için JSON dosyası oluştur | ||
| for (const [language, translations] of Object.entries(this.translations)) { | ||
| const filePath = path.join(this.localesDir, `${language}.json`); | ||
| fs.writeFileSync(filePath, JSON.stringify(translations, null, 2)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Catch JSON write errors.
Heavy sauce is falling from the sky: even though you write the files in a for-loop, an unhandled write error for one language could disrupt the entire operation. Consider try-catch blocks inside the loop to distinguish partial failures from total meltdown.
Server/service/translationService.js
Outdated
| this.apiToken = "ddf8d5fdbe1baa12bb3b5519b639d00a"; | ||
| this.projectId = "757606"; | ||
| this.baseUrl = 'https://api.poeditor.com/v2'; |
There was a problem hiding this comment.
Avoid storing tokens in source code.
Arms are heavy, but we can keep everything steady by storing your tokens in environment variables instead of committing them directly to code. This helps prevent accidental leaks of crucial credentials like your POEditor API token.
- this.apiToken = "ddf8d5fdbe1baa12bb3b5519b639d00a";
- this.projectId = "757606";
+ this.apiToken = process.env.POEDITOR_API_TOKEN || "fallbackToken";
+ this.projectId = process.env.POEDITOR_PROJECT_ID || "fallbackProjectId";📝 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.
| this.apiToken = "ddf8d5fdbe1baa12bb3b5519b639d00a"; | |
| this.projectId = "757606"; | |
| this.baseUrl = 'https://api.poeditor.com/v2'; | |
| this.apiToken = process.env.POEDITOR_API_TOKEN || "fallbackToken"; | |
| this.projectId = process.env.POEDITOR_PROJECT_ID || "fallbackProjectId"; | |
| this.baseUrl = 'https://api.poeditor.com/v2'; |
Server/utils/formattedKey.js
Outdated
| export const formattedKey = (key) => { | ||
| return key.toLowerCase() | ||
| .split('_') | ||
| .map((word, index) => | ||
| index === 0 ? word : word.charAt(0).toUpperCase() + word.slice(1) | ||
| ) | ||
| .join(''); | ||
| }; |
There was a problem hiding this comment.
Yo dawg, we need some input validation up in here!
The function's got no chill when it comes to handling edge cases. We should add validation for null, undefined, and empty strings before they make our app lose its cool.
Here's how we can make it bulletproof:
export const formattedKey = (key) => {
+ if (!key || typeof key !== 'string') {
+ throw new Error('Key must be a non-empty string');
+ }
return key.toLowerCase()
.split('_')
.map((word, index) =>
index === 0 ? word : word.charAt(0).toUpperCase() + word.slice(1)
)
.join('');
};📝 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.
| export const formattedKey = (key) => { | |
| return key.toLowerCase() | |
| .split('_') | |
| .map((word, index) => | |
| index === 0 ? word : word.charAt(0).toUpperCase() + word.slice(1) | |
| ) | |
| .join(''); | |
| }; | |
| export const formattedKey = (key) => { | |
| if (!key || typeof key !== 'string') { | |
| throw new Error('Key must be a non-empty string'); | |
| } | |
| return key.toLowerCase() | |
| .split('_') | |
| .map((word, index) => | |
| index === 0 ? word : word.charAt(0).toUpperCase() + word.slice(1) | |
| ) | |
| .join(''); | |
| }; |
| @@ -328,7 +328,7 @@ const getUptimeDetailsById = async (req) => { | |||
| const { monitorId } = req.params; | |||
| const monitor = await Monitor.findById(monitorId); | |||
| if (monitor === null || monitor === undefined) { | |||
| throw new Error(errorMessages.DB_FIND_MONITOR_BY_ID(monitorId)); | |||
| throw new Error(errorMessages.DB_FIND_MONITOR_BY_ID(monitorId, req.language)); | |||
There was a problem hiding this comment.
Inconsistent error message internationalization
While some error messages use req.language for internationalization, others still use the old format without language support. For example, in the getMonitorById method, the error message doesn't use the language parameter.
Ensure consistent use of language parameter:
const getMonitorById = async (monitorId) => {
try {
const monitor = await Monitor.findById(monitorId);
if (monitor === null || monitor === undefined) {
- const error = new Error(errorMessages.DB_FIND_MONITOR_BY_ID(monitorId));
+ const error = new Error(errorMessages.DB_FIND_MONITOR_BY_ID(monitorId, req.language));
error.status = 404;
throw error;
}
// ... rest of the code ...
}
};Apply this pattern to all error messages in the module for consistent internationalization support.
Also applies to: 721-721
| msg: successMessages.QUEUE_GET_METRICS(req.language), | ||
| data: metrics, |
There was a problem hiding this comment.
Add language parameter validation.
The controller assumes req.language is always present. Consider adding validation to handle cases where it's missing.
getMetrics = async (req, res, next) => {
try {
+ if (!req.language) {
+ throw new Error('Language parameter is required');
+ }
const metrics = await this.jobQueue.getMetrics();
res.success({
msg: successMessages.QUEUE_GET_METRICS(req.language),
data: metrics,
});Also applies to: 28-29, 41-41, 53-53
| const error = new Error(errorMessages.NO_AUTH_TOKEN(req.language)); | ||
| error.status = 401; |
There was a problem hiding this comment.
Ensure consistent i18n error handling.
Error messages for JWT verification are not using i18n, while token validation errors are. This inconsistency could lead to mixed language responses.
jwt.verify(parsedToken, jwtSecret, (err, decoded) => {
if (err) {
logger.error({
message: err.message,
service: SERVICE_NAME,
method: "verifySuperAdmin",
stack: err.stack,
- details: errorMessages.INVALID_AUTH_TOKEN,
+ details: errorMessages.INVALID_AUTH_TOKEN(req.language),
});
return res
.status(401)
- .json({ success: false, msg: errorMessages.INVALID_AUTH_TOKEN });
+ .json({ success: false, msg: errorMessages.INVALID_AUTH_TOKEN(req.language) });
}Also applies to: 28-29
Server/index.js
Outdated
| app.use(express.json()); | ||
| app.use(helmet()); | ||
| app.use(languageMiddleware); |
There was a problem hiding this comment.
💡 Codebase verification
Yo, this middleware's got some weak spots! 🍝
The implementation's pretty basic and might cause problems:
- No validation against supported languages - it'll accept anything!
- Hardcoded 'en' default without configuration options
- Missing error handling for malformed headers
- No documentation of what languages we actually support
You might wanna beef this up with:
- A supported languages configuration
- Proper validation and error handling
- Configurable default language
🔗 Analysis chain
Hold up, we need to verify this middleware's default behavior! 🎤
The languageMiddleware is placed correctly in the middleware chain, but we should verify its language detection logic and default language handling.
Let's check the middleware implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Analyze language middleware implementation
echo "Checking language middleware implementation..."
rg -A 10 "languageMiddleware.*=|export.*languageMiddleware" --type js
echo "Checking for language-related configuration..."
rg -i "default.*language|supported.*language" --type js
Length of output: 1010
Server/index.js
Outdated
| @@ -175,6 +178,7 @@ const startApp = async () => { | |||
| const networkService = new NetworkService(axios, ping, logger, http, Docker, net); | |||
| const statusService = new StatusService(db, logger); | |||
| const notificationService = new NotificationService(emailService, db, logger); | |||
| const translationService = new TranslationService(logger); | |||
There was a problem hiding this comment.
💡 Codebase verification
Yo dawg, we got a security leak! 🚨 Mom's spaghetti is all over the code!
Found hardcoded POEditor credentials in the service that should be moved to environment variables:
- Move
apiToken: "ddf8d5fdbe1baa12bb3b5519b639d00a"to environment variable - Move
projectId: "757606"to environment variable
Just like how NGROK_AUTH_TOKEN is handled, these sensitive credentials should be configured through environment variables to maintain security best practices.
🔗 Analysis chain
Yo dawg, we might be missing some secret sauce here! 🤔
The TranslationService initialization looks clean, but where's the POEditor configuration mentioned in the PR objectives? We should verify if any API keys or endpoints need to be passed here.
Let's check if there are any POEditor-related environment variables or configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for POEditor-related configuration
echo "Searching for POEditor configuration..."
rg -i "poeditor|translation.*key|translation.*config" --type js
Length of output: 681
Script:
#!/bin/bash
# Search for environment variables and config patterns in translation service
echo "Checking TranslationService implementation..."
cat Server/service/translationService.js
echo -e "\nChecking for environment variable usage..."
rg -i "process\.env\." Server/service/translationService.js
echo -e "\nChecking similar service configurations..."
rg -i "process\.env\." Server/index.js
Length of output: 6235
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
Large PR Notification
Dear contributor,
Thank you for your substantial contribution to this project. LlamaPReview has detected that this Pull Request contains a large volume of changes, which exceeds our current processing capacity.
Details:
- PR and related contents total size: Approximately 285,007 characters
- Current limit: 50,000 characters
Next steps:
- Consider breaking this PR into smaller, more focused changes if possible.
- For manual review, please reach out to your team members or maintainers.
We appreciate your understanding and commitment to improving this project. Your contributions are valuable, and we want to ensure they receive the attention they deserve.
LlamaPReview is continuously evolving to better serve the community. Share your thoughts on handling large PRs in our GitHub Discussions - your feedback helps us improve and expand our capabilities.
If you have any questions or need assistance, our community and support team are here to help.
Best regards,
LlamaPReview Team
There was a problem hiding this comment.
This is a great start!
I believe it would make more sense to create a StringService or something along those lines to handle providing strings rather than pass req.language to every single part of the application that requires a string.
How is error handling and fallback handled? If the i18n service fails entirely will the app still run as expected? It looks like it defaults to en, that should be the case in event of any error. Please elaborate on this 👍
Server/utils/messages.js
Outdated
| AUTH_INVITE_NOT_FOUND: "Invite not found", | ||
| UNAUTHORIZED: (language) => getTranslatedMessage('UNAUTHORIZED', language), | ||
| AUTH_ADMIN_EXISTS: (language) => getTranslatedMessage('AUTH_ADMIN_EXISTS', language), | ||
| AUTH_INVITE_NOT_FOUND: (language) => getTranslatedMessage('AUTH_INVITE_NOT_FOUND', language), | ||
|
|
There was a problem hiding this comment.
Rather than have to pass req.language to every single piece of the backend code why don't we create a StringService and just instantiate it once with the users preferred language and handle getting the required strings there?
That would be a much cleaner and maintainable solution. Adding a layer of abstraction will also allow us to swap to i18n tools if we ever need to.
Server/service/translationService.js
Outdated
| @@ -0,0 +1,187 @@ | |||
| import axios from 'axios'; | |||
There was a problem hiding this comment.
Please use the NetworkService for all network requests
Server/service/translationService.js
Outdated
| import axios from 'axios'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { formattedKey } from '../utils/formattedKey.js'; |
There was a problem hiding this comment.
Please inject all depdencies rather than importing to facilitate testing
| stack: error.stack | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
What happens to the application if initialization here fails entirely? Does it still run?
There was a problem hiding this comment.
If we cannot fetch the translations from POEditor API, it will fetch the translations from local_en.json in local as defaults
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
Server/controllers/authController.js (2)
Line range hint
471-498: This delete flow is heavy! Let's break it down into smaller chunks!The deleteUser method is doing too much and has multiple responsibilities. Consider extracting the monitor cleanup logic into a separate method.
+ async cleanupMonitors(monitors) { + if (!monitors?.length) return; + await Promise.all( + monitors.map(async (monitor) => { + await this.jobQueue.deleteJob(monitor); + await this.db.deleteChecks(monitor._id); + await this.db.deletePageSpeedChecksByMonitorId(monitor._id); + await this.db.deleteNotificationsByMonitorId(monitor._id); + }) + ); + } deleteUser = async (req, res, next) => { try { const token = getTokenFromHeaders(req.headers); const decodedToken = jwt.decode(token); const { email } = decodedToken; const user = await this.db.getUserByEmail(email, req.language); const result = await this.db.getMonitorsByTeamId({ params: { teamId: user.teamId }, }); if (user.role.includes("superadmin")) { - result?.monitors.length > 0 && - (await Promise.all( - result.monitors.map(async (monitor) => { - await this.jobQueue.deleteJob(monitor); - await this.db.deleteChecks(monitor._id); - await this.db.deletePageSpeedChecksByMonitorId(monitor._id); - await this.db.deleteNotificationsByMonitorId(monitor._id); - }) - )); + await this.cleanupMonitors(result?.monitors);
Email subjects need internationalization! 🍝
While the codebase has excellent i18n coverage, there are two hardcoded English email subjects that should be moved to the translation system:
- "Welcome to Uptime Monitor" in registerUser method
- "Checkmate Password Reset" in requestRecovery method
🔗 Analysis chain
Line range hint
1-519: Overall architecture check: Let's verify the i18n implementation!Let's verify that all user-facing messages are properly internationalized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hardcoded English strings that might need i18n rg -g '*.js' -g '*.ts' -i "subject:|message:|msg:|error:" Server/ | grep -i '"[^"]*[a-z][^"]*"'Length of output: 2965
Script:
#!/bin/bash # Check for direct string literals in authController.js rg -g 'authController.js' '"[^"]*"' Server/controllers/ # Check the message utilities cat Server/utils/messages.js # Check email subjects rg -g '*.js' 'subject:|buildAndSendEmail' Server/controllers/authController.jsLength of output: 11941
♻️ Duplicate comments (1)
Server/service/networkService.js (1)
107-107:⚠️ Potential issueThere's vomit on his sweater already - hardcoded languages!
Multiple instances of hardcoded 'en' language in success and error messages defeat the purpose of i18n support.
The language should be passed through the job object:
-pingResponse.message = successMessages.PING_SUCCESS('en'); +pingResponse.message = successMessages.PING_SUCCESS(job.data.language || 'en'); -throw new Error(errorMessages.DOCKER_NOT_FOUND('en')); +throw new Error(errorMessages.DOCKER_NOT_FOUND(job.data.language || 'en')); -dockerResponse.message = successMessages.DOCKER_SUCCESS('en'); +dockerResponse.message = successMessages.DOCKER_SUCCESS(job.data.language || 'en'); -portResponse.message = successMessages.PORT_SUCCESS('en'); +portResponse.message = successMessages.PORT_SUCCESS(job.data.language || 'en');Also applies to: 254-254, 276-276, 330-330
🧹 Nitpick comments (4)
Server/service/stringService.js (1)
299-302: String interpolation is inconsistent.The string interpolation in
getDeletedCountuses a different pattern ({count}) compared togetDbFindMonitorById(${monitorId}).Standardize the string interpolation pattern across all methods:
getDeletedCount(count) { return this.translationService.getTranslation('deletedCount') - .replace('{count}', count); + .replace('${count}', count); }Server/service/translationService.js (1)
189-203: Translation fallback chain needs improvement.The current fallback mechanism is too simple and might not handle all edge cases.
Enhance the fallback mechanism:
getTranslation(key) { let language = this._language; try { - return this.translations[language]?.[key] || this.translations['en']?.[key] || key; + // Try exact language match + if (this.translations[language]?.[key]) { + return this.translations[language][key]; + } + + // Try language without region code (e.g., 'en-US' -> 'en') + const baseLanguage = language.split('-')[0]; + if (language !== baseLanguage && this.translations[baseLanguage]?.[key]) { + return this.translations[baseLanguage][key]; + } + + // Fallback to English + if (this.translations['en']?.[key]) { + return this.translations['en'][key]; + } + + // Log missing translation + this.logger.warn({ + message: `Translation missing for key: ${key}`, + service: 'TranslationService', + method: 'getTranslation', + language, + key + }); + + return key;Server/controllers/authController.js (2)
157-160: Straight fire error handling! But let's make it even hotter!The error handling for incorrect password could be more informative by including the service name and method.
- const error = new Error(this.stringService.authIncorrectPassword); - error.status = 401; + const error = new Error(this.stringService.authIncorrectPassword); + error.status = 401; + error.service = SERVICE_NAME; + error.method = 'loginUser'; next(error);
225-226: Yo, these error messages are straight fire! But let's DRY them up!The ternary operator for error messages could be moved to the stringService for better maintainability.
- const errorMessage = - refreshErr.name === "TokenExpiredError" - ? this.stringService.expiredAuthToken - : this.stringService.invalidAuthToken; + const errorMessage = this.stringService.getAuthTokenError(refreshErr.name);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Docker/dist/docker-compose.yamlis excluded by!**/dist/**
📒 Files selected for processing (8)
Server/controllers/authController.js(20 hunks)Server/index.js(5 hunks)Server/middleware/languageMiddleware.js(1 hunks)Server/service/networkService.js(7 hunks)Server/service/stringService.js(1 hunks)Server/service/translationService.js(1 hunks)Server/tests/controllers/queueController.test.js(8 hunks)Server/utils/locales_en.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Server/tests/controllers/queueController.test.js
- Server/utils/locales_en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/index.js
🧰 Additional context used
🪛 Biome (1.9.4)
Server/service/stringService.js
[error] 6-6: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🔇 Additional comments (3)
Server/service/stringService.js (1)
4-12:⚠️ Potential issueSingleton implementation needs improvement.
The singleton pattern implementation has a potential issue - the constructor can return undefined if StringService.instance is falsy.
Apply this diff to fix the singleton implementation:
class StringService { static SERVICE_NAME = "StringService"; + static instance = null; constructor(translationService) { if (StringService.instance) { return StringService.instance; } + if (!translationService) { + throw new Error('TranslationService is required'); + } this.translationService = translationService; this._language = 'en'; // default language StringService.instance = this; + return this; }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 6-6: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
Server/controllers/authController.js (2)
Line range hint
106-108: Mom's spaghetti alert! Email welcome template needs i18n love!The welcome email subject is still hardcoded in English while the success messages are internationalized.
Also applies to: 120-122
Line range hint
366-383: Password reset template needs that i18n sauce!Similar to the welcome email, the password reset email subject is hardcoded in English.
| getDbFindMonitorById(monitorId) { | ||
| return this.translationService.getTranslation('dbFindMonitorById') | ||
| .replace('${monitorId}', monitorId); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error handling is missing in string interpolation methods.
The string replacement operations lack error handling for undefined or null values.
Add error handling:
getDbFindMonitorById(monitorId) {
+ if (!monitorId) {
+ this.logger.error({
+ message: 'Monitor ID is required',
+ service: StringService.SERVICE_NAME,
+ method: 'getDbFindMonitorById'
+ });
+ return this.translationService.getTranslation('dbFindMonitorById');
+ }
return this.translationService.getTranslation('dbFindMonitorById')
.replace('${monitorId}', monitorId);
}Committable suggestion skipped: line range outside the PR's diff.
| const languageMiddleware = (stringService, translationService) => (req, res, next) => { | ||
| const acceptLanguage = req.headers['accept-language'] || 'en'; | ||
| const language = acceptLanguage.split(',')[0].slice(0, 2).toLowerCase(); | ||
|
|
||
| translationService.setLanguage(language); | ||
| stringService.setLanguage(language); | ||
|
|
||
| next(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Language handling needs improvement.
The current implementation has several potential issues:
- No validation of language codes
- No error handling for invalid headers
- No fallback chain for language selection
Enhance the language handling:
const languageMiddleware = (stringService, translationService) => (req, res, next) => {
+ const SUPPORTED_LANGUAGES = ['en', 'es', 'fr']; // Add your supported languages
+ const DEFAULT_LANGUAGE = 'en';
+
const acceptLanguage = req.headers['accept-language'] || 'en';
- const language = acceptLanguage.split(',')[0].slice(0, 2).toLowerCase();
+
+ let language;
+ try {
+ // Parse the Accept-Language header properly
+ const preferred = acceptLanguage
+ .split(',')
+ .map(lang => {
+ const [code, priority = '1.0'] = lang.trim().split(';q=');
+ return { code: code.slice(0, 2).toLowerCase(), priority: parseFloat(priority) };
+ })
+ .sort((a, b) => b.priority - a.priority);
+
+ // Find the first supported language
+ language = preferred.find(lang => SUPPORTED_LANGUAGES.includes(lang.code))?.code || DEFAULT_LANGUAGE;
+ } catch (error) {
+ language = DEFAULT_LANGUAGE;
+ }
translationService.setLanguage(language);
stringService.setLanguage(language);
+ req.language = language; // Store for use in routes
next();
};📝 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.
| const languageMiddleware = (stringService, translationService) => (req, res, next) => { | |
| const acceptLanguage = req.headers['accept-language'] || 'en'; | |
| const language = acceptLanguage.split(',')[0].slice(0, 2).toLowerCase(); | |
| translationService.setLanguage(language); | |
| stringService.setLanguage(language); | |
| next(); | |
| }; | |
| const languageMiddleware = (stringService, translationService) => (req, res, next) => { | |
| const SUPPORTED_LANGUAGES = ['en', 'es', 'fr']; // Add your supported languages | |
| const DEFAULT_LANGUAGE = 'en'; | |
| const acceptLanguage = req.headers['accept-language'] || 'en'; | |
| let language; | |
| try { | |
| // Parse the Accept-Language header properly | |
| const preferred = acceptLanguage | |
| .split(',') | |
| .map(lang => { | |
| const [code, priority = '1.0'] = lang.trim().split(';q='); | |
| return { code: code.slice(0, 2).toLowerCase(), priority: parseFloat(priority) }; | |
| }) | |
| .sort((a, b) => b.priority - a.priority); | |
| // Find the first supported language | |
| language = preferred.find(lang => SUPPORTED_LANGUAGES.includes(lang.code))?.code || DEFAULT_LANGUAGE; | |
| } catch (error) { | |
| language = DEFAULT_LANGUAGE; | |
| } | |
| translationService.setLanguage(language); | |
| stringService.setLanguage(language); | |
| req.language = language; // Store for use in routes | |
| next(); | |
| }; |
There was a problem hiding this comment.
Continuing good work!
I haven't had a chance to do a thorough review yet, but I still see req.language being used throughout the controllers.
I believe the idea is to move all of the strings to StringService, that way we should never need to use req.language and all the strings will be in one location.
I'll have a more thorough look as soon as possible!
Server/controllers/authController.js
Outdated
| @@ -76,7 +77,7 @@ class AuthController { | |||
| // If superAdmin exists, a token should be attached to all further register requests | |||
| const superAdminExists = await this.db.checkSuperadmin(req, res); | |||
| if (superAdminExists) { | |||
| await this.db.getInviteTokenAndDelete(inviteToken); | |||
| await this.db.getInviteTokenAndDelete(inviteToken, req.language); | |||
There was a problem hiding this comment.
We shouldn't need req.language anymore, you can pass a reference to StringService to MongoDB when it is instantiated
Server/controllers/authController.js
Outdated
| @@ -176,7 +177,7 @@ class AuthController { | |||
| userWithoutPassword.avatarImage = user.avatarImage; | |||
|
|
|||
| return res.success({ | |||
| msg: successMessages.AUTH_LOGIN_USER, | |||
| msg: successMessages.AUTH_LOGIN_USER(req.language), | |||
There was a problem hiding this comment.
Same for these, these strings should now all be provided by the StringService
I have prepared the stringService architecture. In places where we will use translations, we can retrieve texts using:
If this implementation is okay for you, I will replace the text occurrences accordingly. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Server/controllers/authController.js (2)
81-81:⚠️ Potential issueYo dawg, we got some spaghetti code here! 🍝
Based on the past review comments,
req.languageshould be removed as strings should be provided by theStringService. This change needs to be consistent across all success messages.Replace all instances of
successMessages.*(req.language)withstringServicecalls:- successMessages.AUTH_CREATE_USER(req.language) + stringService.authCreateUser - successMessages.AUTH_LOGIN_USER(req.language) + stringService.authLoginUser // ... and so on for all other success messagesAlso applies to: 90-90, 121-121, 182-182, 251-251, 321-321, 343-343, 389-389, 420-420, 453-453, 507-507, 519-519
154-154:⚠️ Potential issueYo, these DB calls need cleanup! 🧹
All calls to
getUserByEmailanddeleteUserare still passingreq.language. According to past review comments, this should be handled by theStringServicein the MongoDB layer.Remove the language parameter from DB calls:
- await this.db.getUserByEmail(email, req.language) + await this.db.getUserByEmail(email) - await this.db.deleteUser(user._id, req.language) + await this.db.deleteUser(user._id)Also applies to: 304-304, 372-372, 477-477, 504-504
🧹 Nitpick comments (1)
Server/controllers/authController.js (1)
142-142: StringService initialization is lit! 🔥Good job getting the StringService from the registry. However, this is being done in multiple methods. Consider initializing it once in the constructor.
Move the initialization to the constructor:
constructor(db, settingsService, emailService, jobQueue) { this.db = db; this.settingsService = settingsService; this.emailService = emailService; this.jobQueue = jobQueue; + this.stringService = ServiceRegistry.get(StringService.SERVICE_NAME); }Also applies to: 206-206, 273-273
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/controllers/authController.js(22 hunks)Server/index.js(5 hunks)
🔇 Additional comments (5)
Server/index.js (4)
77-79: Yo dawg, these imports are clean! 🎤The imports are well-organized and follow a logical order.
182-183: Yo, service initialization is on point! 🔥The services are initialized in the correct order, with
translationServicebeing created beforestringServicesince it's a dependency.
204-206: Services registered like mom's spaghetti! 🍝Both services are properly registered in the ServiceRegistry, maintaining the same order as their initialization.
280-280: Middleware placement is straight fire! 🔥The language middleware is correctly placed after basic middleware (cors, json, helmet) but before routes.
Server/controllers/authController.js (1)
159-161: Error messages are now properly localized! 🎯Good job using
stringServicefor error messages. This is the correct approach for localization.Also applies to: 214-218, 229-230, 286-290, 310-314
I see many references to Here's what I'd like to see in the service:
eg should now be or something along those lines. The exact implementation here is up to you, but it must be decoupled and have minimal impact on the rest of the application. As well don't worry about updating tests, I will be rewriting all the tests in the future so no need to expend effort there. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
Server/middleware/verifyOwnership.js (1)
20-22: 🛠️ Refactor suggestionSpaghetti code alert: Let's DRY up these error messages! 🍝
Error creation logic is duplicated in three places. Let's extract this into a helper function.
+ const createError = (message, status) => { + const error = new Error(message); + error.status = status; + return error; + }; - const error = new Error(stringService.verifyOwnerNotFound); - error.status = 404; + const error = createError(stringService.verifyOwnerNotFound, 404); - const error = new Error(stringService.verifyOwnerUnauthorized); - error.status = 403; + const error = createError(stringService.verifyOwnerUnauthorized, 403);Also applies to: 28-30, 38-40
Server/db/mongo/modules/recoveryModule.js (1)
44-44:⚠️ Potential issueSecurity check needed: User enumeration possible! 🍝
Different error messages based on various conditions could enable user enumeration attacks. Let's implement consistent timing and generic error messages.
+ const GENERIC_RECOVERY_ERROR = 'Recovery process could not be completed'; + const sleep = (ms) => new Promise(resolve => setTimeout(resolve, ms)); + + const handleRecoveryError = async () => { + await sleep(500); // Add consistent timing + return new Error(GENERIC_RECOVERY_ERROR); + }; - throw new Error(stringService.dbTokenNotFound); + throw await handleRecoveryError(); - throw new Error(stringService.dbUserNotFound); + throw await handleRecoveryError(); - throw new Error(stringService.dbResetPasswordBadMatch); + throw await handleRecoveryError();Also consider adding rate limiting to prevent brute force attempts:
import rateLimit from 'express-rate-limit'; const recoveryLimiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 5 // limit each IP to 5 requests per windowMs });Also applies to: 63-63, 68-68
Server/controllers/maintenanceWindowController.js (1)
117-117:⚠️ Potential issueHold up, something's not right with this message key!
The method
getMaintenanceWindowsByMonitorIdis usingmaintenanceWindowGetByUserwhich doesn't match the method's purpose.Apply this diff to fix the message key:
- msg: this.stringService.maintenanceWindowGetByUser, + msg: this.stringService.maintenanceWindowGetByMonitor,Server/controllers/monitorController.js (1)
395-398:⚠️ Potential issueYo, this notification creation needs some error handling!
The Promise.all for notification creation could silently fail without proper error handling.
Apply this diff to add error handling:
await Promise.all( notifications && notifications.map(async (notification) => { notification.monitorId = editedMonitor._id; - await this.db.createNotification(notification); + try { + await this.db.createNotification(notification); + } catch (error) { + logger.error({ + message: `Failed to create notification for monitor ${editedMonitor._id}`, + service: SERVICE_NAME, + method: "editMonitor", + stack: error.stack + }); + throw error; + } }) );🧰 Tools
🪛 Biome (1.9.4)
[error] 394-398: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (8)
Server/service/networkService.js (1)
276-276: Clean up that boolean logic, it's making me nervous!The static analysis tool correctly identified unnecessary use of boolean literals.
-dockerResponse.status = response?.State?.Status === "running" ? true : false; +dockerResponse.status = response?.State?.Status === "running";🧰 Tools
🪛 Biome (1.9.4)
[error] 276-276: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Server/middleware/handleErrors.js (1)
7-9: Yo dawg, let's optimize that service initialization! 🍝The
StringServiceis being initialized on every error. Move it outside the handler function to avoid unnecessary initialization overhead.+ const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); const handleErrors = (error, req, res, next) => { const status = error.status || 500; - const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); const message = error.message || stringService.friendlyError;Server/controllers/settingsController.js (1)
18-18: Mom's spaghetti: Performance optimization needed! 🍝Using the
deleteoperator can impact performance. Let's create a new object without the sensitive field instead.- const settings = { ...(await this.settingsService.getSettings()) }; - delete settings.jwtSecret; + const { jwtSecret, ...settings } = await this.settingsService.getSettings();🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Server/db/mongo/modules/recoveryModule.js (1)
71-71: Vomit on his sweater already: Clear sensitive data! 🍝After password reset, ensure we clear sensitive data from memory.
user.password = newPassword; await user.save(); + user.password = undefined; // Clear sensitive data + newPassword = undefined; await RecoveryToken.deleteMany({ email: recoveryToken.email });Server/middleware/verifyJWT.js (1)
21-21: Consistent error message handling through StringService, nice work!All error messages are now properly sourced from StringService:
- Authentication token errors
- Refresh token errors
- Invalid token messages
However, there's one thing that's making me nervous, like mom's spaghetti...
Consider caching the StringService instance in a local variable to avoid multiple ServiceRegistry.get() calls:
const verifyJWT = (req, res, next) => { + const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); // ... rest of the code if (!token) { - const error = new Error(ServiceRegistry.get(StringService.SERVICE_NAME).noAuthToken); + const error = new Error(stringService.noAuthToken); } // ... rest of the code }; function handleExpiredJwtToken(req, res, next) { - const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); // ... rest of the code }Also applies to: 29-29, 47-47, 65-65, 81-82, 92-92
Server/controllers/authController.js (2)
16-17: Yo dawg, let's validate that StringService before we drop the beat! 🎵Add service validation in the constructor to fail fast if the service isn't registered.
constructor(db, settingsService, emailService, jobQueue) { + const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); + if (!stringService) { + throw new Error('StringService not registered'); + } this.db = db; this.settingsService = settingsService; this.emailService = emailService; this.jobQueue = jobQueue; - this.stringService = ServiceRegistry.get(StringService.SERVICE_NAME); + this.stringService = stringService; }Also applies to: 25-25
90-93: These response objects be looking like they lost themselves in the music! 🎵Let's standardize the response structure across all endpoints:
-return res.success({ - msg: this.stringService.authCreateUser, -}); +return res.success({ + msg: this.stringService.authCreateUser, + data: null // Explicitly set to null when no data +});Apply this pattern to all success responses for consistent API responses.
Also applies to: 121-123, 181-187, 249-251, 318-321, 340-343, 386-388, 417-419, 450-453, 504-506, 516-519
Server/db/mongo/modules/monitorModule.js (1)
563-634: These aggregation pipelines got vomit on their sweater already! 🎵The aggregation pipelines are complex and repetitive. Let's refactor them into reusable functions.
+const createCheckLookupStage = (checkType, limit) => ({ + $lookup: { + from: checkType, + let: { monitorId: "$_id" }, + pipeline: [ + { + $match: { + $expr: { $eq: ["$monitorId", "$$monitorId"] }, + }, + }, + { $sort: { createdAt: -1 } }, + ...(limit ? [{ $limit: limit }] : []), + ], + as: `${checkType}checks`, + }, +}); ...(limit ? [ - { - $lookup: { - from: "checks", - let: { monitorId: "$_id" }, - pipeline: [ - { - $match: { - $expr: { $eq: ["$monitorId", "$$monitorId"] }, - }, - }, - { $sort: { createdAt: -1 } }, - ...(limit ? [{ $limit: limit }] : []), - ], - as: "standardchecks", - }, - }, + createCheckLookupStage("standard", limit), ] : []),Apply this pattern to all check type lookups for better maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Docker/dist/docker-compose.yamlis excluded by!**/dist/**
📒 Files selected for processing (21)
Server/controllers/authController.js(18 hunks)Server/controllers/checkController.js(7 hunks)Server/controllers/inviteController.js(3 hunks)Server/controllers/maintenanceWindowController.js(7 hunks)Server/controllers/monitorController.js(14 hunks)Server/controllers/queueController.js(4 hunks)Server/controllers/settingsController.js(2 hunks)Server/controllers/statusPageController.js(3 hunks)Server/db/mongo/modules/inviteModule.js(3 hunks)Server/db/mongo/modules/monitorModule.js(7 hunks)Server/db/mongo/modules/recoveryModule.js(5 hunks)Server/db/mongo/modules/statusPageModule.js(3 hunks)Server/db/mongo/modules/userModule.js(5 hunks)Server/middleware/handleErrors.js(1 hunks)Server/middleware/isAllowed.js(3 hunks)Server/middleware/verifyJWT.js(6 hunks)Server/middleware/verifyOwnership.js(3 hunks)Server/middleware/verifySuperAdmin.js(3 hunks)Server/service/jobQueue.js(5 hunks)Server/service/networkService.js(8 hunks)Server/utils/locales_en.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- Server/db/mongo/modules/inviteModule.js
- Server/controllers/queueController.js
- Server/db/mongo/modules/statusPageModule.js
- Server/utils/locales_en.json
- Server/middleware/isAllowed.js
- Server/controllers/statusPageController.js
- Server/db/mongo/modules/userModule.js
- Server/service/jobQueue.js
- Server/middleware/verifySuperAdmin.js
🧰 Additional context used
🪛 Biome (1.9.4)
Server/service/networkService.js
[error] 276-276: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
Server/controllers/settingsController.js
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (7)
Server/service/networkService.js (1)
103-103: Straight fire! Clean message handling with StringService! 🔥Great job replacing all hardcoded messages with StringService. This aligns perfectly with the PR objectives and makes the code more maintainable.
Also applies to: 109-109, 256-256, 273-273, 278-278, 326-326, 332-332
Server/middleware/verifyJWT.js (1)
4-4: StringService initialization looks good, eh!The service initialization follows the singleton pattern using ServiceRegistry. Beauty!
Also applies to: 17-17
Server/controllers/inviteController.js (2)
10-12: Clean StringService integration, eh!The imports and service initialization look proper in the constructor.
Also applies to: 20-20
72-72: Success messages properly localized, beauty!Both
inviteIssuedandinviteVerifiedmessages are correctly sourced from StringService.Also applies to: 92-92
Server/controllers/checkController.js (2)
15-16: StringService setup looks mint!Clean import and initialization of StringService in the constructor.
Also applies to: 24-24
41-41: Consistent message localization across all methods, eh!All success messages are properly sourced from StringService:
- checkCreate
- checkGet
- checkDelete
- checkUpdateTTL
Also applies to: 62-62, 82-82, 102-102, 122-122, 149-149
Server/controllers/monitorController.js (1)
443-444: Clean ternary operator for pause/resume messages!Nice use of conditional messaging based on the monitor's state.
| checksWithResponseTime.length | ||
| : 0, |
There was a problem hiding this comment.
Yo, this average calculation's got me weak in the knees! 🎵
The avgResponseTime calculation is returning the count instead of the actual average. This is a critical bug.
-avgResponseTime:
- checksWithResponseTime.length > 0
- ? checksWithResponseTime.length
- : 0,
+avgResponseTime:
+ checksWithResponseTime.length > 0
+ ? checksWithResponseTime.reduce((sum, check) => sum + check.responseTime, 0) / checksWithResponseTime.length
+ : 0,📝 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.
| checksWithResponseTime.length | |
| : 0, | |
| checksWithResponseTime.length > 0 | |
| ? checksWithResponseTime.reduce((sum, check) => sum + check.responseTime, 0) / checksWithResponseTime.length | |
| : 0, |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Server/index.js (1)
207-209:⚠️ Potential issueYo, we need some error handling here! 🍝
The translation service initialization could fail if POEditor is down. Add error handling:
- await translationService.initialize(); + try { + await translationService.initialize(); + } catch (error) { + logger.error({ + message: "Failed to initialize translation service", + service: SERVICE_NAME, + method: "startApp", + stack: error.stack + }); + throw error; + }Server/service/networkService.js (3)
35-41:⚠️ Potential issueMom's spaghetti moment: Missing POEditor credentials should stop the show!
The service continues running even when POEditor credentials are missing. This could lead to runtime errors when POEditor methods are called.
Apply this fix to fail fast:
if (!this.apiToken || !this.projectId) { - this.logger.error({ + const error = new Error('POEditor API token or project ID is missing in environment variables'); + error.service = this.SERVICE_NAME; + error.method = 'constructor'; + this.logger.error({ message: 'POEditor API token or project ID is missing in environment variables', service: this.SERVICE_NAME, method: 'constructor' - }); + }); + throw error; }
99-105:⚠️ Potential issueThere's vomit on his sweater already - hardcoded messages!
Several messages are still hardcoded instead of using the StringService:
- "No response" and "Success" in the ping response
- "Failed to fetch Docker container information" and "Docker container status fetched successfully" in the docker response
Apply this fix:
-pingResponse.message = "No response"; +pingResponse.message = this.stringService.pingNoResponse; -pingResponse.message = "Success"; +pingResponse.message = this.stringService.pingSuccess; -dockerResponse.message = error.reason || "Failed to fetch Docker container information"; +dockerResponse.message = error.reason || this.stringService.dockerFetchFailed; -dockerResponse.message = "Docker container status fetched successfully"; +dockerResponse.message = this.stringService.dockerSuccess;Also applies to: 269-274
380-467: 🛠️ Refactor suggestionKnees weak, arms are heavy: POEditor API calls need some safety!
The POEditor integration needs improvements:
- Add retry mechanism with exponential backoff
- Add input validation
- Consolidate error handling
Here's how to implement these improvements:
+ async retryWithBackoff(operation, maxRetries = 3) { + for (let i = 0; i < maxRetries; i++) { + try { + return await operation(); + } catch (error) { + if (i === maxRetries - 1) throw error; + await new Promise(resolve => setTimeout(resolve, Math.pow(2, i) * 1000)); + } + } + } + handlePoEditorError(error, method) { + error.service = this.SERVICE_NAME; + error.method = method; + throw error; + } async exportPoEditorTranslations(language) { try { + if (!language || typeof language !== 'string') { + throw new Error('Invalid language parameter'); + } const params = new URLSearchParams(); params.append('api_token', this.apiToken); params.append('id', this.projectId); params.append('language', language); params.append('type', 'key_value_json'); - const exportResponse = await this.axios.post(...); + const exportResponse = await this.retryWithBackoff(() => + this.axios.post(`${NetworkService.POEDITOR_BASE_URL}/projects/export`, params, { + headers: { + 'Content-Type': 'application/x-www-form-urlencoded' + } + }) + ); const { url } = exportResponse.data.result; - const translationsResponse = await this.axios.get(url); + const translationsResponse = await this.retryWithBackoff(() => this.axios.get(url)); return translationsResponse.data; } catch (error) { - error.service = this.SERVICE_NAME; - error.method = "exportPoEditorTranslations"; - throw error; + this.handlePoEditorError(error, "exportPoEditorTranslations"); } }Apply similar changes to other POEditor methods.
🧹 Nitpick comments (1)
Server/service/networkService.js (1)
272-272: Simplify the boolean expression, yo!The boolean expression can be simplified by directly using the comparison result.
-dockerResponse.status = response?.State?.Status === "running" ? true : false; +dockerResponse.status = response?.State?.Status === "running";🧰 Tools
🪛 Biome (1.9.4)
[error] 272-272: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/index.js(6 hunks)Server/service/networkService.js(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
Server/service/networkService.js
[error] 272-272: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (3)
Server/index.js (3)
77-79: Yo, these imports are clean! 🎵The new i18n-related imports are properly organized with other service imports.
163-167: Yo dawg, we need to verify the translation config! 🎤The translation service initialization looks clean, but make sure:
- POEditor API credentials are loaded from environment variables
- No hardcoded API tokens or project IDs in the TranslationService
#!/bin/bash # Check for hardcoded credentials in TranslationService echo "Checking TranslationService for hardcoded credentials..." rg -i "apiToken|projectId|poeditor" Server/service/translationService.js echo "Checking environment variable usage..." rg "process\.env\." Server/service/translationService.js
282-282: Yo, this middleware needs some validation! 🎤The language middleware implementation might need strengthening:
- Add validation for supported languages
- Configure default language through environment variables
- Add error handling for malformed headers
#!/bin/bash # Check language middleware implementation echo "Checking language middleware..." rg -A 10 "languageMiddleware" Server/middleware/languageMiddleware.js echo "Checking for language configuration..." rg "DEFAULT_LANGUAGE|SUPPORTED_LANGUAGES" Server/config/
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Server/service/translationService.js (3)
7-13: Add parameter validation in constructor.Mom's spaghetti is getting cold! Let's add some validation to ensure our dependencies are properly injected:
constructor(logger, networkService) { + if (!logger || !networkService) { + throw new Error('Required dependencies missing: logger and networkService must be provided'); + } this.logger = logger; this.networkService = networkService; this.translations = {}; this._language = 'en'; this.localesDir = path.join(process.cwd(), 'locales'); }
186-200: Enhance translation key validation and formatting.Arms are heavy, but we can make this translation retrieval more robust!
getTranslation(key) { + if (!key || typeof key !== 'string') { + this.logger.warn({ + message: `Invalid translation key: ${key}`, + service: 'TranslationService', + method: 'getTranslation' + }); + return ''; + } let language = this._language; try { - return this.translations[language]?.[key] || this.translations['en']?.[key] || key; + const translation = this.translations[language]?.[key] || this.translations['en']?.[key]; + return translation || this.handleMissingTranslation(key); } catch (error) { this.logger.error({ message: error.message, service: 'TranslationService', method: 'getTranslation', stack: error.stack }); - return key; + return this.handleMissingTranslation(key); } } + + private handleMissingTranslation(key) { + this.logger.warn({ + message: `Missing translation for key: ${key}`, + service: 'TranslationService', + method: 'handleMissingTranslation' + }); + return key; + }
216-244: Implement rate limiting for POEditor API calls.Palms are sweaty, but we need to respect API rate limits!
Consider implementing a rate limiter for POEditor API calls to avoid hitting rate limits. You can use a token bucket algorithm or similar rate limiting mechanism:
+ private async rateLimiter() { + const RATE_LIMIT = 10; // requests per second + const now = Date.now(); + if (this.lastRequestTime && (now - this.lastRequestTime) < (1000 / RATE_LIMIT)) { + await new Promise(resolve => + setTimeout(resolve, (1000 / RATE_LIMIT) - (now - this.lastRequestTime)) + ); + } + this.lastRequestTime = Date.now(); + } async addTermsToPOEditor(terms) { try { if (!terms.length) return; + await this.rateLimiter(); const response = await this.networkService.addPoEditorTerms(terms);
Server/service/translationService.js
Outdated
| async syncTermsWithPOEditor() { | ||
| try { | ||
| const utilsPath = path.join(process.cwd(), 'utils'); | ||
| const utilsFilePath = path.join(utilsPath, 'locales_en.json'); | ||
| const enTranslations = JSON.parse(fs.readFileSync(utilsFilePath, 'utf8')); | ||
| const localTerms = Object.keys(enTranslations) | ||
| .map(term => term); | ||
|
|
||
| const poeditorTerms = await this.getTermsFromPOEditor(); | ||
|
|
||
| const newTerms = localTerms?.filter(term => !poeditorTerms?.includes(term)); | ||
|
|
||
|
|
||
| this.logger.info({ | ||
| message: `Comparison results - New terms found: ${newTerms.length}`, | ||
| sampleNewTerms: newTerms.slice(0, 5), | ||
| service: 'TranslationService', | ||
| method: 'syncTermsWithPOEditor' | ||
| }); | ||
|
|
||
| if (newTerms.length > 0) { | ||
| const formattedTerms = newTerms.map(term => ({ | ||
| [term]: enTranslations[term] || '', | ||
| })); | ||
|
|
||
| await this.addTermsToPOEditor(formattedTerms); | ||
|
|
||
| } else { | ||
| this.logger.info({ | ||
| message: 'No new terms found to synchronize', | ||
| service: 'TranslationService', | ||
| method: 'syncTermsWithPOEditor' | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| this.logger.error({ | ||
| message: error.message, | ||
| service: 'TranslationService', | ||
| method: 'syncTermsWithPOEditor', | ||
| stack: error.stack | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance term synchronization validation and batching.
Heavy sauce is falling from the sky, but we can make this term sync more robust!
async syncTermsWithPOEditor() {
try {
+ const BATCH_SIZE = 100; // POEditor might have limits
const utilsPath = path.join(process.cwd(), 'utils');
const utilsFilePath = path.join(utilsPath, 'locales_en.json');
+
+ if (!fs.existsSync(utilsFilePath)) {
+ throw new Error('English translations file not found');
+ }
+
const enTranslations = JSON.parse(fs.readFileSync(utilsFilePath, 'utf8'));
const localTerms = Object.keys(enTranslations)
.map(term => term);
const poeditorTerms = await this.getTermsFromPOEditor();
+
+ if (!Array.isArray(poeditorTerms)) {
+ throw new Error('Invalid response from POEditor');
+ }
const newTerms = localTerms?.filter(term => !poeditorTerms?.includes(term));
if (newTerms.length > 0) {
- const formattedTerms = newTerms.map(term => ({
- [term]: enTranslations[term] || '',
- }));
-
- await this.addTermsToPOEditor(formattedTerms);
+ // Process terms in batches
+ for (let i = 0; i < newTerms.length; i += BATCH_SIZE) {
+ const batch = newTerms.slice(i, i + BATCH_SIZE);
+ const formattedTerms = batch.map(term => ({
+ [term]: enTranslations[term] || '',
+ }));
+ await this.addTermsToPOEditor(formattedTerms);
+ }📝 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.
| async syncTermsWithPOEditor() { | |
| try { | |
| const utilsPath = path.join(process.cwd(), 'utils'); | |
| const utilsFilePath = path.join(utilsPath, 'locales_en.json'); | |
| const enTranslations = JSON.parse(fs.readFileSync(utilsFilePath, 'utf8')); | |
| const localTerms = Object.keys(enTranslations) | |
| .map(term => term); | |
| const poeditorTerms = await this.getTermsFromPOEditor(); | |
| const newTerms = localTerms?.filter(term => !poeditorTerms?.includes(term)); | |
| this.logger.info({ | |
| message: `Comparison results - New terms found: ${newTerms.length}`, | |
| sampleNewTerms: newTerms.slice(0, 5), | |
| service: 'TranslationService', | |
| method: 'syncTermsWithPOEditor' | |
| }); | |
| if (newTerms.length > 0) { | |
| const formattedTerms = newTerms.map(term => ({ | |
| [term]: enTranslations[term] || '', | |
| })); | |
| await this.addTermsToPOEditor(formattedTerms); | |
| } else { | |
| this.logger.info({ | |
| message: 'No new terms found to synchronize', | |
| service: 'TranslationService', | |
| method: 'syncTermsWithPOEditor' | |
| }); | |
| } | |
| } catch (error) { | |
| this.logger.error({ | |
| message: error.message, | |
| service: 'TranslationService', | |
| method: 'syncTermsWithPOEditor', | |
| stack: error.stack | |
| }); | |
| } | |
| } | |
| async syncTermsWithPOEditor() { | |
| try { | |
| const BATCH_SIZE = 100; // POEditor might have limits | |
| const utilsPath = path.join(process.cwd(), 'utils'); | |
| const utilsFilePath = path.join(utilsPath, 'locales_en.json'); | |
| if (!fs.existsSync(utilsFilePath)) { | |
| throw new Error('English translations file not found'); | |
| } | |
| const enTranslations = JSON.parse(fs.readFileSync(utilsFilePath, 'utf8')); | |
| const localTerms = Object.keys(enTranslations) | |
| .map(term => term); | |
| const poeditorTerms = await this.getTermsFromPOEditor(); | |
| if (!Array.isArray(poeditorTerms)) { | |
| throw new Error('Invalid response from POEditor'); | |
| } | |
| const newTerms = localTerms?.filter(term => !poeditorTerms?.includes(term)); | |
| this.logger.info({ | |
| message: `Comparison results - New terms found: ${newTerms.length}`, | |
| sampleNewTerms: newTerms.slice(0, 5), | |
| service: 'TranslationService', | |
| method: 'syncTermsWithPOEditor' | |
| }); | |
| if (newTerms.length > 0) { | |
| // Process terms in batches | |
| for (let i = 0; i < newTerms.length; i += BATCH_SIZE) { | |
| const batch = newTerms.slice(i, i + BATCH_SIZE); | |
| const formattedTerms = batch.map(term => ({ | |
| [term]: enTranslations[term] || '', | |
| })); | |
| await this.addTermsToPOEditor(formattedTerms); | |
| } | |
| } else { | |
| this.logger.info({ | |
| message: 'No new terms found to synchronize', | |
| service: 'TranslationService', | |
| method: 'syncTermsWithPOEditor' | |
| }); | |
| } | |
| } catch (error) { | |
| this.logger.error({ | |
| message: error.message, | |
| service: 'TranslationService', | |
| method: 'syncTermsWithPOEditor', | |
| stack: error.stack | |
| }); | |
| } | |
| } |
Server/service/translationService.js
Outdated
| this.logger.error({ | ||
| message: error.message, | ||
| service: 'TranslationService', | ||
| method: 'getTranslation', | ||
| stack: error.stack | ||
| }); | ||
| return key; | ||
| } | ||
| } | ||
|
|
||
| async getTermsFromPOEditor() { | ||
| try { | ||
| return await this.networkService.getPoEditorTerms(); | ||
| } catch (error) { | ||
| this.logger.error({ | ||
| message: error.message, | ||
| service: 'TranslationService', | ||
| method: 'getTermsFromPOEditor', | ||
| stack: error.stack | ||
| }); | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| async addTermsToPOEditor(terms) { | ||
| try { | ||
| if (!terms.length) return; | ||
|
|
||
| const response = await this.networkService.addPoEditorTerms(terms); | ||
|
|
||
| if (response.response?.status === 'fail') { | ||
| throw new Error(response.response.message || 'Failed to add terms to POEditor'); | ||
| } | ||
|
|
||
| this.logger.info({ | ||
| message: `${terms.length} new terms added to POEditor`, | ||
| service: 'TranslationService', | ||
| method: 'addTermsToPOEditor', | ||
| response: response | ||
| }); | ||
|
|
||
| return response; | ||
| } catch (error) { | ||
| this.logger.error({ | ||
| message: `Failed to add terms to POEditor: ${error.message}`, | ||
| service: 'TranslationService', | ||
| method: 'addTermsToPOEditor', | ||
| stack: error.stack, | ||
| terms: terms | ||
| }); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| async syncTermsWithPOEditor() { | ||
| try { | ||
| const utilsPath = path.join(process.cwd(), 'utils'); | ||
| const utilsFilePath = path.join(utilsPath, 'locales_en.json'); | ||
| const enTranslations = JSON.parse(fs.readFileSync(utilsFilePath, 'utf8')); | ||
| const localTerms = Object.keys(enTranslations) | ||
| .map(term => term); | ||
|
|
||
| const poeditorTerms = await this.getTermsFromPOEditor(); | ||
|
|
||
| const newTerms = localTerms?.filter(term => !poeditorTerms?.includes(term)); | ||
|
|
||
|
|
||
| this.logger.info({ | ||
| message: `Comparison results - New terms found: ${newTerms.length}`, | ||
| sampleNewTerms: newTerms.slice(0, 5), | ||
| service: 'TranslationService', | ||
| method: 'syncTermsWithPOEditor' | ||
| }); | ||
|
|
||
| if (newTerms.length > 0) { | ||
| const formattedTerms = newTerms.map(term => ({ | ||
| [term]: enTranslations[term] || '', | ||
| })); | ||
|
|
||
| await this.addTermsToPOEditor(formattedTerms); | ||
|
|
||
| } else { | ||
| this.logger.info({ | ||
| message: 'No new terms found to synchronize', | ||
| service: 'TranslationService', | ||
| method: 'syncTermsWithPOEditor' | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| this.logger.error({ | ||
| message: error.message, | ||
| service: 'TranslationService', | ||
| method: 'syncTermsWithPOEditor', | ||
| stack: error.stack | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export default TranslationService; |
There was a problem hiding this comment.
💡 Codebase verification
Yo! Your spaghetti's missing its sauce - TranslationService needs tests!
While your project's got the testing framework cooked up nice with Mocha, Chai, and c8 coverage (found in package.json), and most services are well-tested (check out those Server/tests/services/), the TranslationService is standing alone in the kitchen without any tests!
- Create
Server/tests/services/translationService.test.jsto match the existing test structure
🔗 Analysis chain
Add comprehensive unit tests.
Mom's spaghetti needs testing! The TranslationService requires comprehensive unit tests to ensure reliability:
Consider adding tests for:
- Translation loading and fallback mechanisms
- Language switching
- POEditor integration
- Error handling scenarios
- File operations
Would you like me to generate a test suite template?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test files
echo "Checking for existing test files..."
fd "test.*translationService" --type f
# Verify test coverage tools
echo "Checking for test coverage configuration..."
fd "jest.config|nyc.config|coverage" --type f
Length of output: 266
Script:
#!/bin/bash
# Check for any test files in Server directory
echo "Checking for test files in Server..."
fd "test|spec" --type f --full-path Server/
# Check package.json for test configuration
echo "Checking package.json for test setup..."
fd "package.json" --type f --exec cat {} \; | grep -A 5 -B 5 '"test":'
# Check for any testing-related dependencies
echo "Checking for test dependencies..."
fd "package.json" --type f --exec cat {} \; | grep -A 10 '"devDependencies":'
Length of output: 2653
There was a problem hiding this comment.
This is looking better, nice work!
All dependencies should be injected via constructor and not imported, each component should be totally decoupled from all other components.
A controller should not depend on a service that is imported. It should interact with the service via a reference it gets from the constructor.
This greatly facilitates testing and modularity of the application.
For example if this translation library breaks all I have to do is replace string service with a totally different implementation.
So long as a service fulfils the same API no other class should care how the service is implemented.
Nice work, thanks for making changes!
Server/controllers/authController.js
Outdated
| import jwt from "jsonwebtoken"; | ||
| import { getTokenFromHeaders, tokenType } from "../utils/utils.js"; | ||
| import crypto from "crypto"; | ||
| import { handleValidationError, handleError } from "./controllerUtils.js"; | ||
| const SERVICE_NAME = "authController"; | ||
| import ServiceRegistry from "../service/serviceRegistry.js"; | ||
| import StringService from "../service/stringService.js"; |
There was a problem hiding this comment.
These should be initialized in the root server file and passed to the controllers as a dependency
Server/controllers/authController.js
Outdated
| import jwt from "jsonwebtoken"; | ||
| import { getTokenFromHeaders, tokenType } from "../utils/utils.js"; | ||
| import crypto from "crypto"; | ||
| import { handleValidationError, handleError } from "./controllerUtils.js"; | ||
| const SERVICE_NAME = "authController"; | ||
| import ServiceRegistry from "../service/serviceRegistry.js"; |
There was a problem hiding this comment.
No imports in any classes except the root server directory please.
All dependencies should be injected through the constructor
| @@ -176,7 +178,7 @@ class AuthController { | |||
| userWithoutPassword.avatarImage = user.avatarImage; | |||
|
|
|||
| return res.success({ | |||
| msg: successMessages.AUTH_LOGIN_USER, | |||
| msg: this.stringService.authLoginUser, | |||
There was a problem hiding this comment.
👍 This is what I something like what I was thinking, looks good
| @@ -1,6 +1,7 @@ | |||
| import InviteToken from "../../models/InviteToken.js"; | |||
| import crypto from "crypto"; | |||
| import { errorMessages } from "../../../utils/messages.js"; | |||
| import ServiceRegistry from "../../../service/serviceRegistry.js"; | |||
| import StringService from "../../../service/stringService.js"; | |||
There was a problem hiding this comment.
Again no imports in db models please, everything should be injected to facilitate testing
There was a problem hiding this comment.
I don't understand how to do this for inviteModule. InviteModule is not a class. Can you give an example?
It has been done this way for verifyJWT before.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
Server/index.js (1)
207-209:⚠️ Potential issueYo, we need some error handling here! Mom's spaghetti is all over the place! 🍝
The
translationService.initialize()call could fail silently. Let's wrap it with proper error handling:- await translationService.initialize(); + try { + await translationService.initialize(); + } catch (error) { + logger.error({ + message: "Failed to initialize translation service", + service: SERVICE_NAME, + method: "startApp", + stack: error.stack + }); + process.exit(1); // Fail fast if translation service can't initialize + }
🧹 Nitpick comments (5)
Server/index.js (1)
224-267: Yo, these controller dependencies are getting heavy! 🏋️Each controller is manually injected with StringService. Consider implementing a base controller or dependency injection container to reduce boilerplate:
+ class BaseController { + constructor(stringService) { + this.stringService = stringService; + } + } - class AuthController { + class AuthController extends BaseController { constructor( mongodb, settingsService, emailService, jobQueue, stringService ) { + super(stringService); // ... rest of the initialization } }Server/controllers/settingsController.js (1)
7-11: Yo dawg, let's document these constructor params!The constructor's looking clean, but it's missing some documentation love. Let's help other devs understand what's going on here.
+/** + * @param {Object} db - Database instance + * @param {Object} settingsService - Settings service instance + * @param {Object} stringService - Internationalization service instance + */ constructor(db, settingsService, stringService) {Server/controllers/authController.js (3)
18-23: Constructor looks good, but consider documenting the stringService parameter.The addition of stringService follows the dependency injection pattern correctly.
Add JSDoc for the stringService parameter:
+ * @param {StringService} stringService - Service for handling internationalized strings
119-121: Standardize success response patterns.Response structures vary across methods. Some include only a message, while others include both message and data.
Consider creating a consistent response structure:
+ /** + * @typedef {Object} SuccessResponse + * @property {string} msg - The i18n message + * @property {Object} [data] - Optional response data + */ + /** + * Creates a standardized success response + * @param {string} message - The i18n message key + * @param {Object} [data] - Optional response data + * @returns {SuccessResponse} + */ + const createSuccessResponse = (message, data) => ({ + msg: message, + ...(data && { data }) + });Then use it consistently:
- res.success({ - msg: this.stringService.authLoginUser, - data: { - user: userWithoutPassword, - token: token, - refreshToken: refreshToken, - }, - }); + res.success(createSuccessResponse( + this.stringService.authLoginUser, + { + user: userWithoutPassword, + token: token, + refreshToken: refreshToken, + } + ));Also applies to: 179-185, 247-249, 316-318, 338-340, 384-386, 415-416, 448-450, 502-503, 514-516
Line range hint
32-49: Consider extracting token handling logic to a dedicated service.Token generation, validation, and refresh logic is scattered throughout the controller. This makes it harder to maintain and test.
Consider creating a
TokenService:interface TokenService { generateAccessToken(payload: object): string; generateRefreshToken(): string; validateRefreshToken(token: string): Promise<void>; refreshAccessToken(refreshToken: string, oldAccessToken: string): Promise<string>; }This would simplify the controller methods and make token handling more maintainable.
Also applies to: 203-249
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Server/controllers/authController.js(18 hunks)Server/controllers/checkController.js(7 hunks)Server/controllers/inviteController.js(3 hunks)Server/controllers/maintenanceWindowController.js(7 hunks)Server/controllers/monitorController.js(14 hunks)Server/controllers/queueController.js(4 hunks)Server/controllers/settingsController.js(2 hunks)Server/controllers/statusPageController.js(3 hunks)Server/index.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Server/controllers/maintenanceWindowController.js
- Server/controllers/monitorController.js
🧰 Additional context used
🪛 Biome (1.9.4)
Server/controllers/settingsController.js
[error] 16-16: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
Server/index.js (3)
77-79: Yo dawg, these imports are clean! 🎵The new imports for translation services follow the project's import style consistently.
289-292: Yo, this middleware stack is lit! But let's verify it! 🎤The languageMiddleware is placed correctly after basic middleware, but we should verify:
- Language detection logic
- Default language handling
- Error cases
#!/bin/bash # Analyze language middleware implementation echo "Checking language middleware configuration..." rg -A 10 "export.*languageMiddleware" Server/middleware/languageMiddleware.js echo "Checking for language-related configuration..." rg -i "default.*language|supported.*language" --type js
163-167: Yo, let's make sure these services are wired up right! 🔌The service initialization order looks solid, but let's verify the dependencies are properly configured:
- networkService -> translationService -> stringService chain
- Service registration in the registry
Let's check the service implementations:
✅ Verification successful
Yo dawg, these services are wired up tighter than mom's spaghetti! 🍝
The dependency chain is spot on and the service registration is legit. Everything's flowing smooth like butter on a hot skillet!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check service dependencies and initialization echo "Checking TranslationService constructor..." rg -A 5 "class TranslationService" Server/service/translationService.js echo "Checking StringService constructor..." rg -A 5 "class StringService" Server/service/stringService.jsLength of output: 670
Server/controllers/authController.js (1)
23-23: Verify POEditor integration with StringService.The PR objectives mention POEditor integration, but there's no direct evidence of how translations are loaded into the StringService.
✅ Verification successful
POEditor integration is properly implemented ✨
The StringService correctly integrates with POEditor through TranslationService, which handles:
- Fetching translations from POEditor API
- Automatic syncing of new terms
- Fallback to local files if needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for POEditor integration echo "Searching for POEditor API usage..." rg -l "poeditor" --type js --type ts echo "Searching for translation loading..." rg -l "loadTranslations|getTranslations" --type js --type ts echo "Checking StringService implementation..." ast-grep --pattern 'class StringService { $$$ }'Length of output: 19902
Script:
#!/bin/bash # Examine TranslationService implementation echo "Checking TranslationService implementation..." ast-grep --pattern 'class TranslationService { $$$ }' echo "Checking NetworkService POEditor usage..." rg "poeditor" Server/service/networkService.js -A 5 -B 5Length of output: 20252
| } | ||
|
|
||
| getAppSettings = async (req, res, next) => { | ||
| try { | ||
| const settings = { ...(await this.settingsService.getSettings()) }; | ||
| delete settings.jwtSecret; | ||
| return res.success({ | ||
| msg: successMessages.GET_APP_SETTINGS, | ||
| msg: this.stringService.getAppSettings, |
There was a problem hiding this comment.
Bruh, you forgot to call the method!
You're referencing the method but not calling it. This will return the function reference instead of the actual message.
- msg: this.stringService.getAppSettings,
+ msg: this.stringService.getAppSettings(),📝 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.
| msg: this.stringService.getAppSettings, | |
| msg: this.stringService.getAppSettings(), |
| @@ -35,7 +36,7 @@ class SettingsController { | |||
| const updatedSettings = { ...(await this.settingsService.reloadSettings()) }; | |||
| delete updatedSettings.jwtSecret; | |||
| return res.success({ | |||
| msg: successMessages.UPDATE_APP_SETTINGS, | |||
| msg: this.stringService.updateAppSettings, | |||
There was a problem hiding this comment.
Same deal here, fam! Method needs to be called!
Just like above, you're passing the function reference instead of calling it.
- msg: this.stringService.updateAppSettings,
+ msg: this.stringService.updateAppSettings(),📝 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.
| msg: this.stringService.updateAppSettings, | |
| msg: this.stringService.updateAppSettings(), |
| @@ -42,7 +41,7 @@ class StatusPageController { | |||
| const { url } = req.params; | |||
| const statusPage = await this.db.getStatusPageByUrl(url); | |||
| return res.success({ | |||
| msg: successMessages.STATUS_PAGE_BY_URL, | |||
| msg: this.stringService.statusPageByUrl, | |||
There was a problem hiding this comment.
Same issue with the method call!
Don't forget to actually call the method to get the message.
- msg: this.stringService.statusPageByUrl,
+ msg: this.stringService.statusPageByUrl(),📝 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.
| msg: this.stringService.statusPageByUrl, | |
| msg: this.stringService.statusPageByUrl(), |
| @@ -23,7 +22,7 @@ class StatusPageController { | |||
| try { | |||
| const statusPage = await this.db.createStatusPage(req.body); | |||
| return res.success({ | |||
| msg: successMessages.STATUS_PAGE_CREATE, | |||
| msg: this.stringService.statusPageCreate, | |||
There was a problem hiding this comment.
Heads up! Method call missing here too!
The stringService methods need to be called to get the actual message.
- msg: this.stringService.statusPageCreate,
+ msg: this.stringService.statusPageCreate(),📝 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.
| msg: this.stringService.statusPageCreate, | |
| msg: this.stringService.statusPageCreate(), |
| } | ||
|
|
||
| getMetrics = async (req, res, next) => { | ||
| try { | ||
| const metrics = await this.jobQueue.getMetrics(); | ||
| res.success({ | ||
| msg: successMessages.QUEUE_GET_METRICS, | ||
| msg: this.stringService.queueGetMetrics, |
There was a problem hiding this comment.
Multiple method calls missing + wrong message used!
- All stringService methods need to be called
- getJobs is using the wrong message (queueGetMetrics instead of queueGetJobs)
- msg: this.stringService.queueGetMetrics,
+ msg: this.stringService.queueGetMetrics(),
- msg: this.stringService.queueGetMetrics,
+ msg: this.stringService.queueGetJobs(),
- msg: this.stringService.queueAddJob,
+ msg: this.stringService.queueAddJob(),
- msg: this.stringService.queueObliterate,
+ msg: this.stringService.queueObliterate(),Also applies to: 28-28, 41-41, 53-53
| @@ -66,7 +67,7 @@ class InviteController { | |||
| }); | |||
|
|
|||
| return res.success({ | |||
| msg: successMessages.INVITE_ISSUED, | |||
| msg: this.stringService.inviteIssued, | |||
There was a problem hiding this comment.
Props on the docs, but methods need calls!
Love the detailed JSDoc documentation! Just need to fix these method calls:
- msg: this.stringService.inviteIssued,
+ msg: this.stringService.inviteIssued(),
- msg: this.stringService.inviteVerified,
+ msg: this.stringService.inviteVerified(),Also applies to: 90-90
| @@ -36,7 +36,7 @@ class CheckController { | |||
| const check = await this.db.createCheck(checkData); | |||
|
|
|||
| return res.success({ | |||
| msg: successMessages.CHECK_CREATE, | |||
| msg: this.stringService.checkCreate, | |||
There was a problem hiding this comment.
Last but not least, method calls needed here too!
Same issue throughout this file - all stringService methods need to be called:
- msg: this.stringService.checkCreate,
+ msg: this.stringService.checkCreate(),
- msg: this.stringService.checkGet,
+ msg: this.stringService.checkGet(),
- msg: this.stringService.checkGet,
+ msg: this.stringService.checkGet(),
- msg: this.stringService.checkDelete,
+ msg: this.stringService.checkDelete(),
- msg: this.stringService.checkDelete,
+ msg: this.stringService.checkDelete(),
- msg: this.stringService.checkUpdateTTL,
+ msg: this.stringService.checkUpdateTTL(),Also applies to: 60-60, 80-80, 100-100, 120-120, 147-147
| const error = new Error(this.stringService.authIncorrectPassword); | ||
| error.status = 401; | ||
| next(error); | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error handling patterns across the controller.
Error creation is scattered throughout the code with inconsistent patterns. Some errors include service and method information, while others don't.
Consider creating a helper method in controllerUtils.js:
+ /**
+ * Creates a standardized error with i18n support
+ * @param {string} message - The i18n message key
+ * @param {number} status - HTTP status code
+ * @param {string} service - Service name
+ * @param {string} method - Method name
+ * @returns {Error}
+ */
+ export const createError = (message, status, service, method) => {
+ const error = new Error(message);
+ error.status = status;
+ error.service = service;
+ error.method = method;
+ return error;
+ };Then use it consistently:
- const error = new Error(this.stringService.authIncorrectPassword);
- error.status = 401;
- next(error);
+ next(createError(
+ this.stringService.authIncorrectPassword,
+ 401,
+ SERVICE_NAME,
+ 'loginUser'
+ ));Also applies to: 210-215, 225-231, 281-286, 305-309
There was a problem hiding this comment.
Looks great! Thanks for incoporating those changes.
A few things to clean up:
- Can you move the service init and register to the respecitve sections? They are noted by comments
// Create Services
// Register Services
just to keep things nice and tidy 👍
-
I believe we can delete
messages.jsnow as it is no longer needed -
Can the fallback
locales_en.jsonfile be moved to thelocalesdirectory where the other translations will be? -
A nodemon config is needed to run the server in dev mode, currently it is stuck in a bootloop
Nice work here!
| const networkService = new NetworkService(axios, ping, logger, http, Docker, net); | ||
| const translationService = new TranslationService(logger, networkService); | ||
| const stringService = new StringService(translationService); | ||
| ServiceRegistry.register(StringService.SERVICE_NAME, stringService); |
There was a problem hiding this comment.
Can you move this to the section with the other services? They are delineated with comments
| const networkService = new NetworkService(axios, ping, logger, http, Docker, net); | ||
| const translationService = new TranslationService(logger, networkService); | ||
| const stringService = new StringService(translationService); | ||
| ServiceRegistry.register(StringService.SERVICE_NAME, stringService); |
There was a problem hiding this comment.
Please move this registration to the service registration section
There was a problem hiding this comment.
Please move this registration to the service registration section
I have to do this here because I need to register the String Service before the MongoDB is created.
| stack: error.stack | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is causing a bootloop in dev mode as new files are written to system causing nodemon to restart, could you please include a nodemon config that ignores these files?
{
"ignore": ["locales/*"]
}
should work
| this.networkService = networkService; | ||
| this.translations = {}; | ||
| this._language = 'en'; | ||
| this.localesDir = path.join(process.cwd(), 'locales'); |
There was a problem hiding this comment.
We have two different directories for translations? We have locales_en.json in utils, but POEditor is writing to locales.
These can be in the same directory unless there is some reason they should not?
Server/utils/messages.js
Outdated
| NO_REFRESH_TOKEN: (language) => getTranslatedMessage('NO_REFRESH_TOKEN', language), | ||
| INVALID_REFRESH_TOKEN: (language) => getTranslatedMessage('INVALID_REFRESH_TOKEN', language), | ||
| EXPIRED_REFRESH_TOKEN: (language) => getTranslatedMessage('EXPIRED_REFRESH_TOKEN', language), | ||
| REQUEST_NEW_ACCESS_TOKEN: (language) => getTranslatedMessage('REQUEST_NEW_ACCESS_TOKEN', language), |
There was a problem hiding this comment.
This file is not needed at all anymore, correct? It can be deleted
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
Server/service/translationService.js (3)
7-13: Yo dawg, we need some configuration sauce! 🍝The default language and locales directory are hardcoded. Consider making these configurable through environment variables or settings:
constructor(logger, networkService) { this.logger = logger; this.networkService = networkService; this.translations = {}; - this._language = 'en'; - this.localesDir = path.join(process.cwd(), 'locales'); + this._language = process.env.DEFAULT_LANGUAGE || 'en'; + this.localesDir = process.env.LOCALES_DIR || path.join(process.cwd(), 'locales'); + this.supportedLanguages = process.env.SUPPORTED_LANGUAGES?.split(',') || ['en']; }
76-114: Add some retry sauce to this spaghetti! 🍝The POEditor fetch could use a retry mechanism for transient failures:
async loadTranslations() { let hasError = false; + const MAX_RETRIES = 3; + const RETRY_DELAY = 1000; try { const languages = await this.getLanguages(); for (const language of languages) { + let retries = 0; try { - const translations = await this.exportTranslations(language); + while (retries < MAX_RETRIES) { + try { + const translations = await this.exportTranslations(language); + this.translations[language] = translations; + break; + } catch (error) { + retries++; + if (retries === MAX_RETRIES) throw error; + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)); + } + } - this.translations[language] = translations; } catch (error) { hasError = true;
183-197: Add some interpolation sauce to these strings! 🍝The translation method could support variable interpolation:
- getTranslation(key) { + getTranslation(key, variables = {}) { let language = this._language; try { - return this.translations[language]?.[key] || this.translations['en']?.[key] || key; + let translation = this.translations[language]?.[key] || this.translations['en']?.[key] || key; + return Object.entries(variables).reduce((str, [key, value]) => + str.replace(new RegExp(`{{${key}}}`, 'g'), value), translation); } catch (error) {Server/locales/default.json (2)
5-5: Inconsistent capitalization in password-related keys.Yo dawg, I noticed some inconsistency in the capitalization - "password" is lowercase while "Password" is capitalized. We should keep it consistent across all strings.
- "password": "password", + "password": "Password",Also applies to: 13-13
1-145: Consider adding metadata for i18n management.Lose yourself in the moment! We should add metadata to help manage translations:
- Language code
- Version/last updated timestamp
- Translation progress
{ + "_metadata": { + "languageCode": "en", + "version": "1.0.0", + "lastUpdated": "2025-01-01", + "translationProgress": "100%" + }, "dontHaveAccount": "Don't have account",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Server/controllers/distributedUptimeController.js(0 hunks)Server/index.js(6 hunks)Server/locales/default.json(1 hunks)Server/nodemon.json(1 hunks)Server/service/translationService.js(1 hunks)Server/utils/messages.js(0 hunks)
💤 Files with no reviewable changes (2)
- Server/controllers/distributedUptimeController.js
- Server/utils/messages.js
✅ Files skipped from review due to trivial changes (1)
- Server/nodemon.json
🔇 Additional comments (5)
Server/service/translationService.js (1)
153-181: Fix that bootloop spaghetti! 🍝The file writes are causing nodemon to restart in dev mode. Add a nodemon config:
Create
nodemon.jsonin project root:{ "ignore": ["locales/*", "*/locales/*"] }Server/index.js (3)
163-167: Move that service registration spaghetti! 🍝Move the StringService registration to the services registration section around line 208 to maintain consistency:
- ServiceRegistry.register(StringService.SERVICE_NAME, stringService); // Register services ServiceRegistry.register(JobQueue.SERVICE_NAME, jobQueue); ServiceRegistry.register(MongoDB.SERVICE_NAME, db); ServiceRegistry.register(SettingsService.SERVICE_NAME, settingsService); ServiceRegistry.register(EmailService.SERVICE_NAME, emailService); ServiceRegistry.register(NetworkService.SERVICE_NAME, networkService); ServiceRegistry.register(StatusService.SERVICE_NAME, statusService); ServiceRegistry.register(NotificationService.SERVICE_NAME, notificationService); ServiceRegistry.register(TranslationService.SERVICE_NAME, translationService); + ServiceRegistry.register(StringService.SERVICE_NAME, stringService);
224-269: This controller injection spaghetti is on point! 🍝The StringService is consistently injected across all controllers, maintaining a clean dependency injection pattern.
290-293: Middleware order looking fresh like mom's spaghetti! 🍝The language middleware is correctly placed after basic middleware (cors, json, helmet) and before route handlers.
Server/locales/default.json (1)
33-33: Verify email-related strings for HTML content.You better lose yourself in the email strings! We need to verify if these strings will contain HTML content, as it might affect how they're displayed in email clients.
Also applies to: 34-34, 37-37, 38-38
✅ Verification successful
Yo, these email strings are clean as mom's spaghetti! 🍝
The strings are only used for UI display and won't be interpreted as HTML. All email content is handled separately through dedicated MJML templates in the Server/templates directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for email template files that might use these strings rg -l "authCheckEmailDescription|authCheckEmailResendEmail|authCheckEmailDidntReceiveEmail|authCheckEmailClickToResend" -g "*.{html,ejs,hbs}"Length of output: 143
Script:
#!/bin/bash # Search for email-related files and usage of these strings echo "=== Searching for email-related files ===" fd -e ts -e js -e jsx -e tsx -e json . | grep -i "email" echo -e "\n=== Searching for string usage ===" rg "authCheckEmailDescription" -A 5 -B 5 echo -e "\n=== Checking for email template dependencies ===" fd package.json -x cat {} | grep -i "email"Length of output: 2463
Script:
#!/bin/bash # Check email service implementation echo "=== Email Service Implementation ===" cat Server/service/emailService.js echo -e "\n=== Checking for HTML template files ===" fd . -e html -e ejs -e hbs -e pug -e mjml echo -e "\n=== Checking CheckEmail component ===" cat Client/src/Pages/Auth/CheckEmail.jsxLength of output: 10339
| async exportTranslations(language) { | ||
| try { | ||
| return await this.networkService.exportPoEditorTranslations(language); | ||
| } catch (error) { | ||
| this.logger.error({ | ||
| message: error.message, | ||
| service: 'TranslationService', | ||
| method: 'exportTranslations', | ||
| stack: error.stack | ||
| }); | ||
| return {}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't let this spaghetti fail silently! 🍝
The exportTranslations method returns an empty object on failure, which could mask issues:
async exportTranslations(language) {
try {
return await this.networkService.exportPoEditorTranslations(language);
} catch (error) {
+ if (!this.translations[language]) {
+ throw new Error(`No fallback translations available for ${language}`);
+ }
this.logger.error({
message: error.message,
service: 'TranslationService',
method: 'exportTranslations',
stack: error.stack
});
- return {};
+ return this.translations[language] || {};
}
}📝 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.
| async exportTranslations(language) { | |
| try { | |
| return await this.networkService.exportPoEditorTranslations(language); | |
| } catch (error) { | |
| this.logger.error({ | |
| message: error.message, | |
| service: 'TranslationService', | |
| method: 'exportTranslations', | |
| stack: error.stack | |
| }); | |
| return {}; | |
| } | |
| } | |
| async exportTranslations(language) { | |
| try { | |
| return await this.networkService.exportPoEditorTranslations(language); | |
| } catch (error) { | |
| if (!this.translations[language]) { | |
| throw new Error(`No fallback translations available for ${language}`); | |
| } | |
| this.logger.error({ | |
| message: error.message, | |
| service: 'TranslationService', | |
| method: 'exportTranslations', | |
| stack: error.stack | |
| }); | |
| return this.translations[language] || {}; | |
| } | |
| } |
| "authPasswordMustMatch": "must match", | ||
| "friendlyError": "Something went wrong...", | ||
| "unknownError": "An unknown error occurred", | ||
| "unauthorized": "Unauthorized access", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate unauthorized access messages.
There's vomit on his sweater already! We've got three identical "Unauthorized access" messages with different keys. We should consolidate these into a single reusable message.
"unauthorized": "Unauthorized access",
- "verifyOwnerUnauthorized": "Unauthorized access",
- "authUnauthorized": "Unauthorized access",
+ "verifyOwnerUnauthorized": "{unauthorized}",
+ "authUnauthorized": "{unauthorized}",Also applies to: 70-70, 79-79
| "dbFindMonitorById": "Monitor with id ${monitorId} not found", | ||
| "dbDeleteChecks": "No checks found for monitor with id ${monitorId}", |
There was a problem hiding this comment.
Template literals need to be replaced with placeholders.
Mom's spaghetti! 🍝 These template literals won't work with most i18n libraries. We should use a standardized placeholder format instead.
- "dbFindMonitorById": "Monitor with id ${monitorId} not found",
- "dbDeleteChecks": "No checks found for monitor with id ${monitorId}",
+ "dbFindMonitorById": "Monitor with id {{monitorId}} not found",
+ "dbDeleteChecks": "No checks found for monitor with id {{monitorId}}"📝 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.
| "dbFindMonitorById": "Monitor with id ${monitorId} not found", | |
| "dbDeleteChecks": "No checks found for monitor with id ${monitorId}", | |
| "dbFindMonitorById": "Monitor with id {{monitorId}} not found", | |
| "dbDeleteChecks": "No checks found for monitor with id {{monitorId}}", |
| "monitorStatsById": "Got monitor stats by Id successfully", | ||
| "monitorGetByIdSuccess": "Got monitor by Id successfully", | ||
| "monitorGetByTeamId": "Got monitors by Team Id successfully", | ||
| "monitorGetByUserIdSuccess": "Got monitor for ${userId} successfully", |
There was a problem hiding this comment.
Inconsistent placeholder format.
Knees weak, arms heavy! This string uses a different template literal format than line 33 which uses {{email}}.
- "monitorGetByUserIdSuccess": "Got monitor for ${userId} successfully",
+ "monitorGetByUserIdSuccess": "Got monitor for {{userId}} successfully",📝 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.
| "monitorGetByUserIdSuccess": "Got monitor for ${userId} successfully", | |
| "monitorGetByUserIdSuccess": "Got monitor for {{userId}} successfully", |
I have completed everything except the first item. You can review the changes. 1 - I have to do this here because I need to register the String Service before the MongoDB is created. |
|
Hey @Cihatata, I think @gorkem-bwl has discussed already with you about some changes that will be required for this implementation as we will have to drop API support. Do you want to close this PR and open a new one as the changes will be significant? Or if you wan to keep working on this PR that's fine too. |
Just fyi and to be on the same page with you guys - I'll request a Github integration for our account soon so we'll be using that one moving forward. Let's also wait to hear from me/them as well. |
Describe your changes
i18n support has been added to the backend using POEditor APIs. During the build process, translations registered in POEditor are retrieved. Messages are then returned based on the user's language. At the middleware layer, the user's language is detected from the request headers.
Note
Before merging, POEditor keys must be added to the project.
Issue number
Mention the issue number(s) this PR addresses (e.g., #123).
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.