I18n support for server #1709
I18n support for server #1709
Conversation
![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 137,382 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
|
Caution Review failedThe pull request is closed. WalkthroughThe changes update numerous components by replacing hardcoded message strings with a dynamic, service-based approach. Nearly every controller, module, middleware, and service has been refactored to accept or utilize a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LanguageMiddleware
participant TranslationService
participant StringService
participant Controller
Client->>LanguageMiddleware: Sends request with Accept-Language header
LanguageMiddleware->>TranslationService: Set language based on header
LanguageMiddleware->>StringService: Set language based on header
LanguageMiddleware->>Controller: Pass request along
Controller->>StringService: Retrieve localized messages
Controller->>Client: Send response with dynamic, localized messages
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
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: 16
🔭 Outside diff range comments (5)
Server/db/mongo/modules/statusPageModule.js (1)
146-146: Knees weak, arms heavy: Found a method name mismatch! 🤔The error.method is set to "getStatusPageByUrl" but the actual function name is "getStatusPage".
- error.method = "getStatusPageByUrl"; + error.method = "getStatusPage";Server/db/mongo/modules/recoveryModule.js (1)
1-89:🧩 Analysis chain
Verify that all error messages are defined in the locales! 🎯
Let's ensure all the string keys used in this file are properly defined in the locales directory.
🏁 Script executed:
#!/bin/bash # Search for the string keys in the locales directory echo "Searching for string keys in locales..." for key in "dbTokenNotFound" "dbUserNotFound" "dbResetPasswordBadMatch"; do echo "Checking key: $key" rg -l "$key" locales/ doneLength of output: 784
Let's dig a little deeper into where these i18n strings are defined. Since the locales directory search didn't yield any results, it suggests that the error keys may be defined elsewhere. In this case, they're likely provided by the StringService module imported in the file. To verify this, please run the following script to check for the definitions of "dbTokenNotFound", "dbUserNotFound", and "dbResetPasswordBadMatch" in the file Server/service/stringService.js:
This will help us verify that the error messages used in recoveryModule.js are properly defined.
🏁 Script executed:
#!/bin/bash echo "Searching for error string definitions in Server/service/stringService.js..." for key in "dbTokenNotFound" "dbUserNotFound" "dbResetPasswordBadMatch"; do echo "Checking key: $key" rg -n "$key" Server/service/stringService.js || echo "Key $key not found in Server/service/stringService.js" doneLength of output: 1211
Missing i18n String Definitions for Recovery Error Handling
It appears that none of the expected i18n keys ("dbTokenNotFound", "dbUserNotFound", "dbResetPasswordBadMatch") are defined in either the locales directory or in Server/service/stringService.js. You’ll need to provide these definitions so that the error messages can be properly internationalised.
- The keys were not found in the locales directory.
- The keys are absent within Server/service/stringService.js.
Please update the necessary files to include these strings.
Server/middleware/verifySuperAdmin.js (1)
41-63: Standardize error handling pattern.The error handling pattern differs from other middleware. Let's make it consistent.
- return res - .status(401) - .json({ success: false, msg: stringService.invalidAuthToken }); + const error = new Error(stringService.invalidAuthToken); + error.status = 401; + error.service = SERVICE_NAME; + next(error); + return;Server/controllers/inviteController.js (1)
58-59: Arms are heavy! Don't forget to internationalize the email subjectThe email subject "Welcome to Uptime Monitor" is hardcoded. Consider using stringService for email subjects too.
this.emailService .buildAndSendEmail( "employeeActivationTemplate", { name: firstname, link: `${clientHost}/register/${inviteToken.token}`, }, req.body.email, - "Welcome to Uptime Monitor" + this.stringService.welcomeEmailSubject )Server/db/mongo/modules/userModule.js (1)
141-145: Mom's spaghetti moment: Missing i18n support in error handling!Several functions still have raw error handling without using the string service for internationalization:
- updateUser (lines 141-145)
- deleteTeam (lines 184-187)
- deleteAllOtherUsers (lines 195-198)
- getAllUsers (lines 206-209)
- logoutUser (lines 217-220)
For consistency with the i18n support, these error cases should also use the string service.
Example pattern to follow:
} catch (error) { error.message = stringService.dbOperationFailed; // Add appropriate message key error.service = SERVICE_NAME; error.method = "methodName"; throw error; }Also applies to: 184-187, 195-198, 206-209, 217-220
🧹 Nitpick comments (18)
Server/db/mongo/modules/statusPageModule.js (2)
9-10: Mom's spaghetti moment: Let's optimize this service initialization! 🍝The stringService is initialized on every function call. Consider moving it to module level for better performance:
const SERVICE_NAME = "statusPageModule"; +const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); const createStatusPage = async (statusPageData, image) => { - const stringService = ServiceRegistry.get(StringService.SERVICE_NAME);Also applies to: 25-25
1-162: Lose yourself in the documentation! 📝The i18n implementation looks solid, but let's add some documentation about:
- How the POEditor translations are fetched and integrated
- Where the locale files are stored
- How to add new translations
This will help future contributors understand the i18n workflow.
Server/db/mongo/modules/recoveryModule.js (2)
34-45: Mom's spaghetti moment: Let's make this error handling even better! 🍝While the i18n integration looks good, we could enhance error handling by including more context in the error message.
Consider adding the token (safely truncated) to help with debugging:
- throw new Error(stringService.dbTokenNotFound); + throw new Error(`${stringService.dbTokenNotFound} (Token: ${candidateToken.substring(0, 8)}...)`);
53-69: Knees weak, arms are heavy: Let's optimize this code! 💪The StringService initialization is duplicated between
validateRecoveryTokenandresetPassword. SinceresetPasswordcallsvalidateRecoveryToken, we could optimize this.Consider passing the stringService instance to
validateRecoveryToken:-const validateRecoveryToken = async (req, res) => { - const stringService = serviceRegistry.get(StringService.SERVICE_NAME); +const validateRecoveryToken = async (req, res, stringService) => { + if (!stringService) { + stringService = serviceRegistry.get(StringService.SERVICE_NAME); + } try { // ... rest of the code ... } }; const resetPassword = async (req, res) => { const stringService = serviceRegistry.get(StringService.SERVICE_NAME); try { const newPassword = req.body.password; // Validate token again - const recoveryToken = await validateRecoveryToken(req, res); + const recoveryToken = await validateRecoveryToken(req, res, stringService);Server/controllers/settingsController.js (1)
16-16: Performance anxiety with that delete operator! 🍝Using the delete operator can impact performance. Let's clean up this spaghetti code!
-delete settings.jwtSecret; +const { jwtSecret, ...safeSettings } = settings;🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Server/controllers/queueController.js (1)
19-21: Clean up those redundant returns like cleaning up spilled spaghetti! 🍝The return statements after next(handleError()) are redundant since the function execution stops at next().
} catch (error) { next(handleError(error, SERVICE_NAME, "getMetrics")); - return; }Also applies to: 32-34, 44-46, 56-58
Server/middleware/verifyOwnership.js (1)
15-20: Consider adding error details to the logger.The error logging could be enhanced by including the documentId to help with debugging.
logger.error({ message: stringService.verifyOwnerNotFound, service: SERVICE_NAME, method: "verifyOwnership", + documentId, });Server/middleware/isAllowed.js (1)
12-20: Add request details to error logging.When authentication fails, including request details would help with debugging.
const error = new Error(stringService.noAuthToken); error.status = 401; error.service = SERVICE_NAME; +error.details = { headers: req.headers }; next(error);Server/middleware/verifySuperAdmin.js (1)
1-2: Modernize import statements for consistency.The file mixes require and import statements. Let's standardize to ES modules.
-const jwt = require("jsonwebtoken"); -const logger = require("../utils/logger"); +import jwt from "jsonwebtoken"; +import logger from "../utils/logger";Server/service/translationService.js (2)
73-87: Add type checking and improve fallback handling.The translation retrieval could be more robust with type checking and structured fallbacks.
getTranslation(key) { + if (typeof key !== 'string') { + this.logger.warn({ + message: 'Invalid translation key type', + service: 'TranslationService', + method: 'getTranslation', + key + }); + return key; + } + let language = this._language; + const fallbackLanguage = 'en'; try { - return this.translations[language]?.[key] || this.translations['en']?.[key] || key; + const translation = + this.translations[language]?.[key] || + this.translations[fallbackLanguage]?.[key]; + + if (!translation) { + this.logger.warn({ + message: 'Translation not found', + service: 'TranslationService', + method: 'getTranslation', + key, + language + }); + } + + return translation || key; } catch (error) {
7-12: Consider adding language validation.Add validation for supported languages during initialization.
constructor(logger) { this.logger = logger; this.translations = {}; this._language = 'en'; this.localesDir = path.join(process.cwd(), 'locales'); + this.supportedLanguages = new Set(['en']); }Server/controllers/statusPageController.js (1)
48-51: Yo! Consider wrapping the error message in a proper Error object with i18n supportThe error message is directly passed to the Error constructor. For consistency with the i18n implementation, consider creating a custom error class that handles localized messages.
- const error = new Error(this.stringService.statusPageNotFound); + const error = new LocalizedError(this.stringService.statusPageNotFound); error.status = 404; throw error;Server/middleware/verifyJWT.js (1)
20-26: Knees weak! Let's clean up this error handlingThe error creation pattern is repeated multiple times. Consider creating a helper function to reduce duplication.
+const createAuthError = (message, status, method = '') => { + const error = new Error(message); + error.status = status; + error.service = SERVICE_NAME; + if (method) error.method = method; + return error; +}; if (!token) { - const error = new Error(stringService.noAuthToken); - error.status = 401; - error.service = SERVICE_NAME; - next(error); + next(createAuthError(stringService.noAuthToken, 401)); return; } if (!token.startsWith(TOKEN_PREFIX)) { - const error = new Error(stringService.invalidAuthToken); - error.status = 400; - error.service = SERVICE_NAME; - error.method = "verifyJWT"; - next(error); + next(createAuthError(stringService.invalidAuthToken, 400, "verifyJWT")); return; }Also applies to: 28-35
Server/service/stringService.js (1)
308-311: Standardize string interpolation placeholders.The service uses inconsistent placeholder styles:
{count}vs${monitorId}. Choose one style and stick to it throughout the service.Apply this diff to standardize the placeholders:
- .replace('${monitorId}', monitorId); + .replace('{monitorId}', monitorId);Also applies to: 325-328
Server/locales/en.json (1)
2-146: Yo, these spellings need to be more Canadian, represent your roots!Several words should follow Canadian English spelling conventions:
- "authLoginTitle": "Log In", + "authLoginTitle": "Log In", - "unauthorized": "Unauthorized access", + "unauthorized": "Unauthorised access", - "authInitialize": "Initializing authentication", + "authInitialize": "Initialising authentication"Server/controllers/checkController.js (1)
39-39: Heads up! Don't forget about error messages, fam! 🤔While you've nicely internationalized the success message, consider also translating validation error messages for a consistent user experience.
Server/controllers/maintenanceWindowController.js (1)
49-49: The i18n game is strong, but let's make it stronger! 💪Success messages are well internationalized. Consider also internationalizing:
- Validation error messages
- Database error messages
- JWT verification error messages
This would provide a fully localized experience for users.
Also applies to: 67-67, 93-93, 133-133, 154-154
Server/db/mongo/modules/userModule.js (1)
7-8: Yo dawg, let's optimize these service initializations!The StringService is being initialized multiple times across different functions. We can improve performance by initializing it once at the module level.
Here's a cleaner approach:
import ServiceRegistry from "../../../service/serviceRegistry.js"; import StringService from "../../../service/stringService.js"; const SERVICE_NAME = "userModule"; +const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); -const insertUser = async (...) => { - const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); +const insertUser = async (...) => { try { ... -const getUserByEmail = async (email) => { - const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); +const getUserByEmail = async (email) => { try { ... -const deleteUser = async (userId) => { - const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); +const deleteUser = async (userId) => { try { ...Also applies to: 24-24, 75-75, 157-157
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
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(5 hunks)Server/db/mongo/modules/inviteModule.js(3 hunks)Server/db/mongo/modules/monitorModule.js(8 hunks)Server/db/mongo/modules/recoveryModule.js(5 hunks)Server/db/mongo/modules/statusPageModule.js(4 hunks)Server/db/mongo/modules/userModule.js(5 hunks)Server/index.js(6 hunks)Server/locales/en.json(1 hunks)Server/middleware/handleErrors.js(1 hunks)Server/middleware/isAllowed.js(3 hunks)Server/middleware/languageMiddleware.js(1 hunks)Server/middleware/verifyJWT.js(6 hunks)Server/middleware/verifyOwnership.js(3 hunks)Server/middleware/verifySuperAdmin.js(3 hunks)Server/nodemon.json(1 hunks)Server/service/jobQueue.js(5 hunks)Server/service/networkService.js(6 hunks)Server/service/stringService.js(1 hunks)Server/service/translationService.js(1 hunks)Server/utils/messages.js(0 hunks)
💤 Files with no reviewable changes (1)
- Server/utils/messages.js
✅ Files skipped from review due to trivial changes (1)
- Server/nodemon.json
🧰 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)
Server/service/networkService.js
[error] 264-264: 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/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 (16)
Server/db/mongo/modules/statusPageModule.js (2)
3-4: Yo, these imports are straight fire! 🔥The addition of ServiceRegistry and StringService imports sets up the foundation for i18n support. Clean and focused changes.
54-55: There's vomit on his sweater already: Same optimization needed here! 🎯Similar to createStatusPage, the stringService initialization should be moved to module level.
Server/db/mongo/modules/recoveryModule.js (1)
4-5: Yo, these imports are straight fire! 🔥The addition of
serviceRegistryandStringServiceimports sets up the foundation for i18n support. The service registry pattern is a solid choice for managing global services.Server/middleware/verifyOwnership.js (1)
2-3: LGTM! Service integration looks solid.The integration of StringService for error messages aligns well with the i18n support objectives.
Also applies to: 7-7
Server/service/stringService.js (1)
1-331: LGTM! Comprehensive message coverage.The service provides excellent coverage for various types of messages (auth, maintenance, errors, etc.) and implements a clean interface for accessing translations.
🧰 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/index.js (3)
164-166: LGTM! Proper service initialization order.The initialization sequence is correct:
- Create TranslationService
- Create StringService with TranslationService dependency
- Register services
- Initialize translations before server start
Also applies to: 210-210
297-297: LGTM! Language middleware properly integrated.The language middleware is correctly placed after basic middleware (cors, json, helmet) but before routes, ensuring proper language detection for all requests.
221-269: LGTM! Comprehensive controller updates.All controllers have been properly updated to use StringService, ensuring consistent message handling throughout the application.
Server/service/networkService.js (1)
16-16: LGTM! Constructor updated to support i18n.The stringService parameter has been correctly added to the constructor and properly initialized as a class property.
Also applies to: 33-33
Server/controllers/monitorController.js (1)
26-31: LGTM! Consistent i18n implementation.The changes properly implement i18n support by:
- Adding stringService to the constructor
- Consistently using stringService for all messages
Server/controllers/authController.js (1)
18-24: LGTM! Thorough i18n implementation.The changes properly implement i18n support by:
- Adding stringService to the constructor
- Consistently using stringService for all messages throughout the controller
Server/db/mongo/modules/monitorModule.js (1)
8-9: Yo, these imports are straight fire! 🔥The addition of ServiceRegistry and StringService imports sets up proper dependency management for internationalization.
Server/controllers/checkController.js (2)
19-23: Yo, the constructor changes are on point! 🔥Clean dependency injection of the stringService - that's the way to handle i18n support.
60-60: These string changes are straight fire! 🎯Consistent implementation of i18n across all methods with clear, descriptive message keys:
- checkGet
- checkDelete
- checkUpdateTTL
Also applies to: 80-80, 100-100, 120-120, 147-147
Server/controllers/maintenanceWindowController.js (1)
17-21: Constructor's looking clean as mom's spaghetti! 🍝Consistent with the i18n pattern used across other controllers.
Server/db/mongo/modules/userModule.js (1)
55-55: Verify string keys in POEditor!Let's ensure that the following string keys exist in POEditor for proper i18n support:
- dbUserExists
- dbUserNotFound
Also applies to: 82-82, 162-162
| const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); | ||
| const message = error.message || stringService.friendlyError; | ||
| const service = error.service || stringService.unknownService; |
There was a problem hiding this comment.
Mom's spaghetti moment: Service retrieval needs error handling! 🍝
The service retrieval could throw if the registry isn't properly initialized. Let's catch that spaghetti before it hits the floor!
-const stringService = ServiceRegistry.get(StringService.SERVICE_NAME);
-const message = error.message || stringService.friendlyError;
-const service = error.service || stringService.unknownService;
+let stringService;
+try {
+ stringService = ServiceRegistry.get(StringService.SERVICE_NAME);
+} catch (e) {
+ logger.error('Failed to get StringService from registry:', e);
+ stringService = {
+ friendlyError: 'An unexpected error occurred',
+ unknownService: 'Unknown Service'
+ };
+}
+const message = error.message || stringService.friendlyError;
+const service = error.service || stringService.unknownService;📝 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 stringService = ServiceRegistry.get(StringService.SERVICE_NAME); | |
| const message = error.message || stringService.friendlyError; | |
| const service = error.service || stringService.unknownService; | |
| let stringService; | |
| try { | |
| stringService = ServiceRegistry.get(StringService.SERVICE_NAME); | |
| } catch (e) { | |
| logger.error('Failed to get StringService from registry:', e); | |
| stringService = { | |
| friendlyError: 'An unexpected error occurred', | |
| unknownService: 'Unknown Service' | |
| }; | |
| } | |
| const message = error.message || stringService.friendlyError; | |
| const service = error.service || stringService.unknownService; |
| @@ -25,7 +25,7 @@ class JobQueueController { | |||
| try { | |||
| const jobs = await this.jobQueue.getJobStats(); | |||
| return res.success({ | |||
| msg: successMessages.QUEUE_GET_METRICS, | |||
| msg: this.stringService.queueGetMetrics, | |||
There was a problem hiding this comment.
Yo, these messages are getting mixed up like mom's spaghetti! 🍝
getJobs method is using queueGetMetrics message instead of a job-specific message.
-msg: this.stringService.queueGetMetrics,
+msg: this.stringService.queueGetJobs,📝 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.queueGetMetrics, | |
| msg: this.stringService.queueGetJobs, |
| async loadFromFiles() { | ||
| try { | ||
| if (!fs.existsSync(this.localesDir)) { | ||
| return false; | ||
| } | ||
|
|
||
| const files = fs.readdirSync(this.localesDir).filter(file => file.endsWith('.json')); | ||
|
|
||
| if (files.length === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| for (const file of files) { | ||
| const language = file.replace('.json', ''); | ||
| const filePath = path.join(this.localesDir, file); | ||
| const content = fs.readFileSync(filePath, 'utf8'); | ||
| this.translations[language] = JSON.parse(content); | ||
| } | ||
|
|
||
| this.logger.info({ | ||
| message: 'Translations loaded from files successfully', | ||
| service: 'TranslationService', | ||
| method: 'loadFromFiles' | ||
| }); | ||
|
|
||
| return true; | ||
| } catch (error) { | ||
| this.logger.error({ | ||
| message: error.message, | ||
| service: 'TranslationService', | ||
| method: 'loadFromFiles', | ||
| stack: error.stack | ||
| }); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance translation file loading with validation and caching.
The current implementation could benefit from JSON schema validation and caching.
async loadFromFiles() {
try {
+ // Add caching
+ if (this.isLoaded) {
+ return true;
+ }
+
if (!fs.existsSync(this.localesDir)) {
return false;
}
const files = fs.readdirSync(this.localesDir).filter(file => file.endsWith('.json'));
if (files.length === 0) {
return false;
}
for (const file of files) {
const language = file.replace('.json', '');
const filePath = path.join(this.localesDir, file);
const content = fs.readFileSync(filePath, 'utf8');
+ // Add basic validation
+ const translations = JSON.parse(content);
+ if (typeof translations !== 'object') {
+ throw new Error(`Invalid translation file format: ${file}`);
+ }
this.translations[language] = JSON.parse(content);
}
+ this.isLoaded = true;
this.logger.info({
message: 'Translations loaded from files successfully',
service: 'TranslationService',
method: 'loadFromFiles'
});
return true;| const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); | ||
| try { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mom's spaghetti! Let's optimize these stringService instantiations
The stringService is instantiated separately in each function. Consider moving it to module scope to avoid redundant instantiations.
import ServiceRegistry from "../../../service/serviceRegistry.js";
import StringService from "../../../service/stringService.js";
+const stringService = ServiceRegistry.get(StringService.SERVICE_NAME);
const getInviteToken = async (token) => {
- const stringService = ServiceRegistry.get(StringService.SERVICE_NAME);
// ... rest of the code
};
const getInviteTokenAndDelete = async (token) => {
- const stringService = ServiceRegistry.get(StringService.SERVICE_NAME);
// ... rest of the code
};Also applies to: 73-74
| constructor(translationService) { | ||
| if (StringService.instance) { | ||
| return StringService.instance; | ||
| } | ||
|
|
||
| this.translationService = translationService; | ||
| this._language = 'en'; // default language | ||
| StringService.instance = this; | ||
| } |
There was a problem hiding this comment.
Fix singleton implementation to follow best practices.
The current singleton implementation has an issue with returning a value from the constructor. Consider using a static factory method instead.
Apply this diff to fix the singleton implementation:
- constructor(translationService) {
- if (StringService.instance) {
- return StringService.instance;
- }
-
- this.translationService = translationService;
- this._language = 'en'; // default language
- StringService.instance = this;
- }
+ static instance = null;
+
+ static getInstance(translationService) {
+ if (!StringService.instance) {
+ StringService.instance = new StringService(translationService);
+ }
+ return StringService.instance;
+ }
+
+ constructor(translationService) {
+ this.translationService = translationService;
+ this._language = 'en'; // default language
+ }🧰 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)
| $match: { | ||
| $or: [ | ||
| { name: { $regex: filter, $options: "i" } }, | ||
| { url: { $regex: filter, $options: "i" } }, | ||
| ], | ||
| }, | ||
| ] | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Yo, this MongoDB regex needs some protection, one shot, one opportunity!
The regex pattern in the MongoDB query could be vulnerable to ReDoS (Regular Expression Denial of Service) attacks if user input isn't properly sanitized.
🏁 Script executed:
#!/bin/bash
# Search for other instances of unescaped regex patterns
rg -A 2 '\$regex:.*\$options' Length of output: 599
Regex Vulnerability Risk: Sanitize the 'filter' Input
- The MongoDB query in Server/db/mongo/modules/monitorModule.js directly uses the user-supplied variable "filter" in the $regex operator without any sanitisation.
- The same pattern exists in tests (using hard-coded strings), confirming the approach but highlighting that production code could be exposed to ReDoS if an attacker provides a malicious regex pattern.
- We should escape or validate the regex to prevent potential ReDoS attacks.
| @@ -373,7 +375,7 @@ const getDistributedUptimeDetailsById = 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(this.stringService.dbFindMonitorById(monitorId)); | |||
There was a problem hiding this comment.
Hold up, we got a bug in this line, don't lose yourself!
The this context is incorrect here. In an arrow function, this doesn't bind to the expected scope.
Fix it like this:
- throw new Error(this.stringService.dbFindMonitorById(monitorId));
+ const stringService = ServiceRegistry.get(StringService.SERVICE_NAME);
+ throw new Error(stringService.dbFindMonitorById(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.
| throw new Error(this.stringService.dbFindMonitorById(monitorId)); | |
| const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); | |
| throw new Error(stringService.dbFindMonitorById(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.
Same template literal issue here, don't let it slip!
The ${userId} template literal needs to be updated for consistency.
- "monitorGetByUserIdSuccess": "Got monitor for ${userId} successfully",
+ "monitorGetByUserIdSuccess": "Got monitor for {0} successfully",| "dbFindMonitorById": "Monitor with id ${monitorId} not found", | ||
| "dbDeleteChecks": "No checks found for monitor with id ${monitorId}", |
There was a problem hiding this comment.
Yo, these template literals need proper escaping, don't choke!
The template literals ${monitorId} in JSON strings won't be processed correctly. They need to be handled by the string service.
Update the format to use numbered placeholders:
- "dbFindMonitorById": "Monitor with id ${monitorId} not found",
- "dbDeleteChecks": "No checks found for monitor with id ${monitorId}",
+ "dbFindMonitorById": "Monitor with id {0} not found",
+ "dbDeleteChecks": "No checks found for monitor with id {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.
| "dbFindMonitorById": "Monitor with id ${monitorId} not found", | |
| "dbDeleteChecks": "No checks found for monitor with id ${monitorId}", | |
| "dbFindMonitorById": "Monitor with id {0} not found", | |
| "dbDeleteChecks": "No checks found for monitor with id {0}", |
| @@ -111,7 +112,7 @@ class MaintenanceWindowController { | |||
| ); | |||
|
|
|||
| return res.success({ | |||
| msg: successMessages.MAINTENANCE_WINDOW_GET_BY_USER, | |||
| msg: this.stringService.maintenanceWindowGetByUser, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo, we got a naming inconsistency that's making me nervous! 😰
The message key maintenanceWindowGetByUser seems out of place since the method is getMaintenanceWindowsByMonitorId. Consider renaming to maintenanceWindowGetByMonitor for consistency.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Server/index.js(6 hunks)Server/service/jobQueue.js(6 hunks)
🔇 Additional comments (5)
Server/service/jobQueue.js (2)
19-29: Yo, nice job with constructor injection!The changes align with best practices by using dependency injection for the string service.
Also applies to: 48-48
592-592: Clean as mom's spaghetti sauce!The message key is appropriately used for logging successful obliteration.
Server/index.js (3)
77-79: Yo dawg, these imports are straight fire! 🔥The new imports for i18n support are well-organized and follow a clean separation of concerns:
- TranslationService for managing translations
- languageMiddleware for request processing
- StringService for string management
222-270: Lost yourself in the code? Controllers got the memo! 🎤All controllers have been consistently updated to include StringService. This is a clean implementation that ensures i18n support across all endpoints.
163-166: Mom's spaghetti moment: Initialize services in the right order! 🍝The initialization sequence looks good, but let's verify the service dependencies:
- TranslationService depends on logger ✅
- StringService depends on TranslationService ✅
Let's check if these services are properly initialized in other environments:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any other TranslationService or StringService initializations rg -l "new (Translation|String)Service"Length of output: 57
Service Initialization Verified – Everything's in Order!
All checks confirm that the TranslationService and StringService are initialised correctly in Server/index.js with the proper dependency order. The search script shows no other initialisations, so we're all set.
- TranslationService is correctly instantiated with the logger dependency.
- StringService receives the TranslationService as expected.
- No additional initialisations were found in the repository.
| @@ -455,7 +457,7 @@ class NewJobQueue { | |||
|
|
|||
| if (wasDeleted === true) { | |||
| this.logger.info({ | |||
| message: successMessages.JOB_QUEUE_DELETE_JOB, | |||
| message: this.stringService.jobQueueDeleteJob, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mom's spaghetti moment: Same message key used for success and error!
Using jobQueueDeleteJob for both success and error cases could lead to confusion. Consider using distinct message keys like jobQueueDeleteJobSuccess and jobQueueDeleteJobError.
Apply this diff:
- message: this.stringService.jobQueueDeleteJob,
+ message: this.stringService.jobQueueDeleteJobSuccess,
service: SERVICE_NAME,
method: "deleteJob",
details: `Deleted job ${monitor._id}`,
});
const workerStats = await this.getWorkerStats(queue);
await this.scaleWorkers(workerStats, queue);
} else {
this.logger.error({
- message: this.stringService.jobQueueDeleteJob,
+ message: this.stringService.jobQueueDeleteJobError,
service: SERVICE_NAME,
method: "deleteJob",Also applies to: 469-469
| cors() | ||
| //We will add configuration later | ||
| ); | ||
| app.use(cors()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Vomit on his sweater already: CORS needs configuration! 🤢
Two concerns here:
- CORS configuration is too permissive with no options
- Language middleware is correctly placed before routes
Add CORS configuration:
-app.use(cors());
+app.use(cors({
+ origin: process.env.ALLOWED_ORIGINS?.split(',') || '*',
+ methods: ['GET', 'POST', 'PUT', 'DELETE', 'OPTIONS'],
+ allowedHeaders: ['Content-Type', 'Authorization', 'Accept-Language']
+}));Also applies to: 298-298
| ServiceRegistry.register(TranslationService.SERVICE_NAME, translationService); | ||
|
|
||
| await translationService.initialize(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Palms are sweaty: Ensure translations are loaded before server start! 😰
Good call on initializing TranslationService before starting the server. However, we should handle initialization failures gracefully.
Add error handling for translation initialization:
+try {
await translationService.initialize();
+} catch (error) {
+ logger.error({
+ message: 'Failed to initialize translations',
+ error: error.message,
+ stack: error.stack
+ });
+ process.exit(1);
+}| const networkService = new NetworkService(axios, ping, logger, http, Docker, net, stringService); | ||
| const settingsService = new SettingsService(AppSettings); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Knees weak, arms heavy: NetworkService needs error handling! 💪
The NetworkService and JobQueue now depend on stringService for i18n support, but we should verify error handling for translation failures.
Let's check the error handling in NetworkService:
Also applies to: 194-196
🏁 Script executed:
#!/bin/bash
# Search for error handling in NetworkService
ast-grep --pattern 'try {
$$$
stringService.$$$
$$$
} catch (error) {
$$$
}'Length of output: 86
🏁 Script executed:
#!/bin/bash
echo "Searching for NetworkService class definition and error handling contexts..."
rg -n "class NetworkService" .
for file in $(rg --files-with-matches "class NetworkService"); do
echo "----------------------------------"
echo "File: $file"
echo "---- Try-catch blocks context (NetworkService) ----"
rg -n "try {" -C 5 "$file"
echo "---- stringService references (NetworkService) ----"
rg -n "stringService\." -C 5 "$file"
done
echo "=================================="
echo "Searching for JobQueue class definition and error handling contexts..."
rg -n "class JobQueue" .
for file in $(rg --files-with-matches "class JobQueue"); do
echo "----------------------------------"
echo "File: $file"
echo "---- Try-catch blocks context (JobQueue) ----"
rg -n "try {" -C 5 "$file"
echo "---- stringService references (JobQueue) ----"
rg -n "stringService\." -C 5 "$file"
doneLength of output: 4874
Attention: Enhance error handling for translation failures in NetworkService and JobQueue
After verifying the code in both Server/service/networkService.js and Server/controllers/queueController.js, it appears that while try–catch blocks are used to wrap operations—including those involving stringService—there isn’t dedicated logic to handle failures specifically from the translation (i.e. stringService) calls. If a translation key is missing or stringService encounters an issue, the generic catch simply augments and rethrows the error without offering a fallback message or more nuanced handling.
- In Server/service/networkService.js, stringService is referenced (e.g. this.stringService.dockerNotFound, portFail, portSuccess), and any error is caught in a general catch block that rethrows it with extra metadata (SERVICE_NAME and method).
- In Server/controllers/queueController.js, similar try–catch blocks are present where stringService is used for constructing response messages.
This means that if translation failures occur, they won’t be handled gracefully; instead, errors will bubble up without a dedicated fallback mechanism.
His palms are sweaty, knees weak, arms are heavy—there’s vomit on his sweater already, mom’s spaghetti. In short, consider adding specific error handling or fallback values for translation failures to fortify your i18n support.
There was a problem hiding this comment.
This all looks pretty good to me!
The middleware setting the language on every request seems wasteful to me however, it seems to me it mihgt be a better approach to set the language once on init and then set language again if the user changes their preference?
| const language = acceptLanguage.split(',')[0].slice(0, 2).toLowerCase(); | ||
|
|
||
| translationService.setLanguage(language); | ||
| stringService.setLanguage(language); |
There was a problem hiding this comment.
Is middleware necessary for this? It seems excessive to set the language on every request 🤔
I imagine we can just set this once on intialization, and then we can update it when the user changes their settings?
There was a problem hiding this comment.
If you mean storing the language preference in the database within the user data and retrieving it from there, we can't do that for authentication pages.
If you’re suggesting setting it once and reusing it without rechecking, I’m not sure how to implement that. How would we detect when the language changes?
If you mean storing the language preference in the database within the user data and retrieving it from there, we can't do that for authentication pages. If you’re suggesting setting it once and reusing it without rechecking, I’m not sure how to implement that. How would we detect when the language changes? |
Good point about the login pages, I didn't consider those. As for how to detect changes, the language setting should be stored in the db, so when that setting is updated we can also update the translation service. Let's leave it for now, we can revisit this again later if needed. |
|
@Cihatata can you resolve conflicts here please? There have been some new strings created and some old ones changed etc. Thank you! |
Cihatata
force-pushed
the
i18n-support-backend
branch
from
b0a9c86 to
fddaf8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
Server/db/mongo/modules/monitorModule.js (2)
609-615:⚠️ Potential issueYo, this MongoDB regex needs some protection, one shot, one opportunity!
The regex pattern in the MongoDB query could be vulnerable to ReDoS (Regular Expression Denial of Service) attacks if user input isn't properly sanitized.
378-378:⚠️ Potential issueYo, this context is wrong, don't lose yourself!
The
thiscontext in arrow functions doesn't bind to the expected scope. Fix it like this:- throw new Error(this.stringService.dbFindMonitorById(monitorId)); + const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); + throw new Error(stringService.dbFindMonitorById(monitorId));Server/middleware/handleErrors.js (1)
7-9:⚠️ Potential issueMom's spaghetti moment: Service retrieval needs error handling! 🍝
The service retrieval could throw if the registry isn't properly initialized. Let's catch that spaghetti before it hits the floor!
-const stringService = ServiceRegistry.get(StringService.SERVICE_NAME); -const message = error.message || stringService.friendlyError; -const service = error.service || stringService.unknownService; +let stringService; +try { + stringService = ServiceRegistry.get(StringService.SERVICE_NAME); +} catch (e) { + logger.error('Failed to get StringService from registry:', e); + stringService = { + friendlyError: 'An unexpected error occurred', + unknownService: 'Unknown Service' + }; +} +const message = error.message || stringService.friendlyError; +const service = error.service || stringService.unknownService;Server/service/stringService.js (1)
4-12:⚠️ Potential issueFix singleton implementation to follow best practices! 🍝
The current singleton implementation has an issue with returning a value from the constructor. Let's make it cleaner!
- constructor(translationService) { - if (StringService.instance) { - return StringService.instance; - } - - this.translationService = translationService; - this._language = 'en'; // default language - StringService.instance = this; - } + static instance = null; + + static getInstance(translationService) { + if (!StringService.instance) { + StringService.instance = new StringService(translationService); + } + return StringService.instance; + } + + constructor(translationService) { + this.translationService = translationService; + this._language = 'en'; // default language + }🧰 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/index.js (1)
295-295:⚠️ Potential issueKnees weak, arms heavy: Configure CORS for i18n support! 💪
The current CORS setup is too permissive and doesn't explicitly allow the
Accept-Languageheader needed for i18n.Apply this diff to properly configure CORS:
-app.use(cors()); +app.use(cors({ + origin: process.env.ALLOWED_ORIGINS?.split(',') || '*', + methods: ['GET', 'POST', 'PUT', 'DELETE', 'OPTIONS'], + allowedHeaders: ['Content-Type', 'Authorization', 'Accept-Language'] +}));
🧹 Nitpick comments (5)
Server/controllers/statusPageController.js (1)
66-66: Heads up! Same message key used in different contexts! 👀The
statusPageByUrlmessage is used in bothgetStatusPageandgetStatusPageByUrlmethods. Consider using distinct messages to provide more context-specific feedback.- msg: this.stringService.statusPageByUrl, + msg: this.stringService.statusPageGet,In
getStatusPageByUrl:- msg: this.stringService.statusPageByUrl, + msg: this.stringService.statusPageGetByUrl,Also applies to: 86-86
Server/controllers/settingsController.js (1)
16-17: Knees weak, arms heavy: Performance hit with delete operator! 🏃Using the delete operator can impact performance. Let's make it lighter!
-delete settings.jwtSecret; +settings.jwtSecret = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Server/service/translationService.js (1)
36-71: Enhance translation file loading with validation and caching! 🍝The current implementation could benefit from JSON schema validation and caching.
async loadFromFiles() { try { + // Add caching + if (this.isLoaded) { + return true; + } + if (!fs.existsSync(this.localesDir)) { return false; } const files = fs.readdirSync(this.localesDir).filter(file => file.endsWith('.json')); if (files.length === 0) { return false; } for (const file of files) { const language = file.replace('.json', ''); const filePath = path.join(this.localesDir, file); const content = fs.readFileSync(filePath, 'utf8'); + // Add basic validation + const translations = JSON.parse(content); + if (typeof translations !== 'object') { + throw new Error(`Invalid translation file format: ${file}`); + } this.translations[language] = JSON.parse(content); } + this.isLoaded = true; this.logger.info({ message: 'Translations loaded from files successfully', service: 'TranslationService', method: 'loadFromFiles' }); return true;Server/service/stringService.js (1)
329-330: Template literal confusion in string replacement! 🍝The string replacement uses template literal syntax (
${monitorId}) but with regular string replacement. Let's fix this confusion!- .replace('${monitorId}', monitorId); + .replace('{monitorId}', monitorId);Server/service/networkService.js (1)
264-264: Vomit on his sweater already: Simplify boolean expression! 🤢The ternary operator for boolean assignment is unnecessary.
Apply this diff to simplify the code:
-dockerResponse.status = response?.State?.Status === "running" ? true : false; +dockerResponse.status = response?.State?.Status === "running";🧰 Tools
🪛 Biome (1.9.4)
[error] 264-264: 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 (27)
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(7 hunks)Server/db/mongo/modules/inviteModule.js(3 hunks)Server/db/mongo/modules/monitorModule.js(8 hunks)Server/db/mongo/modules/recoveryModule.js(5 hunks)Server/db/mongo/modules/statusPageModule.js(5 hunks)Server/db/mongo/modules/userModule.js(5 hunks)Server/index.js(6 hunks)Server/locales/en.json(1 hunks)Server/middleware/handleErrors.js(1 hunks)Server/middleware/isAllowed.js(3 hunks)Server/middleware/languageMiddleware.js(1 hunks)Server/middleware/verifyJWT.js(6 hunks)Server/middleware/verifyOwnership.js(3 hunks)Server/middleware/verifySuperAdmin.js(3 hunks)Server/nodemon.json(1 hunks)Server/service/jobQueue.js(6 hunks)Server/service/networkService.js(6 hunks)Server/service/stringService.js(1 hunks)Server/service/translationService.js(1 hunks)Server/utils/messages.js(0 hunks)
💤 Files with no reviewable changes (1)
- Server/utils/messages.js
🚧 Files skipped from review as they are similar to previous changes (15)
- Server/middleware/verifyOwnership.js
- Server/middleware/languageMiddleware.js
- Server/controllers/checkController.js
- Server/nodemon.json
- Server/controllers/queueController.js
- Server/db/mongo/modules/userModule.js
- Server/middleware/verifySuperAdmin.js
- Server/db/mongo/modules/inviteModule.js
- Server/middleware/verifyJWT.js
- Server/db/mongo/modules/statusPageModule.js
- Server/controllers/inviteController.js
- Server/db/mongo/modules/recoveryModule.js
- Server/controllers/maintenanceWindowController.js
- Server/middleware/isAllowed.js
- Server/locales/en.json
🧰 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)
Server/service/networkService.js
[error] 264-264: 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/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 (12)
Server/service/jobQueue.js (3)
19-29: LGTM! Constructor injection implemented correctly.The constructor now properly accepts and initializes the
stringServiceparameter, following the dependency injection pattern.Also applies to: 48-48
460-460: Mom's spaghetti moment: Same message key used for success and error!Using
jobQueueDeleteJobfor both success and error cases could lead to confusion. Consider using distinct message keys.Apply this diff:
- message: this.stringService.jobQueueDeleteJob, + message: this.stringService.jobQueueDeleteJobSuccess, service: SERVICE_NAME, method: "deleteJob", details: `Deleted job ${monitor._id}`, }); const workerStats = await this.getWorkerStats(queue); await this.scaleWorkers(workerStats, queue); } else { this.logger.error({ - message: this.stringService.jobQueueDeleteJob, + message: this.stringService.jobQueueDeleteJobError, service: SERVICE_NAME, method: "deleteJob",Also applies to: 469-469
592-592: LGTM! Proper use of stringService for obliterate message.The obliterate message is correctly using the stringService.
Server/db/mongo/modules/monitorModule.js (2)
316-316: LGTM! The calculation fix is straight fire!The avgResponseTime calculation has been fixed to correctly use checksWithResponseTime.length.
624-698: Yo, these aggregation stages are spittin' straight fire!The MongoDB aggregation pipeline is well-structured with:
- Proper use of $lookup for joining collections
- Efficient sorting and limiting of results
- Clear separation of concerns between different check types
Server/controllers/statusPageController.js (4)
12-15: Yo, these constructor changes are straight fire! 🔥The addition of
stringServicethrough dependency injection is clean and follows solid principles. This sets up the foundation for the i18n support like mom's spaghetti - simple but effective!
1-120: This code's ready to drop some international beats! 🎵The i18n implementation is clean, consistent, and maintains the existing error handling patterns. Just make sure to verify those message keys and consider the suggested message key distinctions.
29-29: Let's make sure our message keys are on point! 🎯We should verify that all message keys exist in our translation files to avoid runtime errors.
Also applies to: 54-54, 66-66, 86-86, 99-99, 111-111
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check for all required message keys KEYS=("statusPageCreate" "statusPageUpdate" "statusPageByUrl" "statusPageByTeamId" "statusPageDelete") echo "Checking translation files for required keys..." for key in "${KEYS[@]}"; do echo "Checking for $key..." fd . locales -e json -x jq -r "if has(\"$key\") then \"✓ $key found in \(.)\" else \"✗ $key missing in \(.)\" end" {} doneLength of output: 1954
Translation Files Verification – Directory Not Found!
The script couldn’t validate the message keys because it couldn’t locate a
localesdirectory. It appears our translation files aren’t where the review comment expects them to be. Could you double-check and confirm if the translations are stored in a different directory (e.g. ani18nfolder) or if there’s another mechanism in place?
49-51: Yo, let's verify these error messages don't drop like mom's spaghetti! 🍝The error message is now internationalized, but we should verify that all required translations exist.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for the statusPageNotFound key in locale files fd . locales -e json -x jq -r 'if has("statusPageNotFound") then "Found in \(.)" else empty end' {}Length of output: 198
Translation Key Verification Requires Manual Intervention
The automated search did not find a directory named “locales” containing the expected translation key. Please manually verify whether the translation forstatusPageNotFoundexists elsewhere in the repository’s i18n files or in a different directory. This verification is critical to ensure that the error message remains fully internationalized and consistent—otherwise, error reporting might drop like mom’s spaghetti!Below is an additional shell script you can run to search the entire repository for the key:
🏁 Script executed:
[run_scripts] #!/bin/bash # Search the entire repository for any occurrence of "statusPageNotFound" rg "statusPageNotFound" .Length of output: 720
Internationalization Verified: Translation key exists
We've confirmed that the translation keystatusPageNotFoundis present, specifically in the fileServer/locales/en.jsonas shown by its value"Status page not found". Additionally, the key is correctly referenced in the code through thestringService. Everything appears to be robust, and the error messages are properly internationalized—no spaghetti dropped here!Server/index.js (1)
163-166: LGTM! Proper service initialization order.The initialization order is correct:
- Create translationService first
- Create stringService with translationService dependency
- Register stringService in ServiceRegistry
This ensures proper dependency injection and service availability.
Server/controllers/monitorController.js (1)
26-31: LGTM! Clean i18n integration.The changes properly integrate i18n support:
- stringService added to constructor
- All messages consistently use stringService
- Proper error handling maintained
Server/controllers/authController.js (1)
18-24: LGTM! Clean i18n integration.The changes properly integrate i18n support:
- stringService added to constructor
- All messages consistently use stringService
- Proper error handling maintained
| msg: this.stringService.getAppSettings, | ||
| data: settings, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Protect against undefined stringService messages! 🍝
Add null checks or default values when accessing stringService messages to avoid potential undefined errors.
-msg: this.stringService.getAppSettings,
+msg: this.stringService?.getAppSettings ?? 'Settings retrieved successfully',
-msg: this.stringService.updateAppSettings,
+msg: this.stringService?.updateAppSettings ?? 'Settings updated successfully',Also applies to: 39-40
| @@ -87,13 +88,13 @@ class NetworkService { | |||
| if (error) { | |||
| pingResponse.status = false; | |||
| pingResponse.code = this.PING_ERROR; | |||
| pingResponse.message = errorMessages.PING_CANNOT_RESOLVE; | |||
| pingResponse.message = "No response"; | |||
There was a problem hiding this comment.
Palms are sweaty: Replace hardcoded messages in requestPing! 😰
The messages "No response" and "Success" are hardcoded instead of using stringService.
Apply this diff to use stringService:
-pingResponse.message = "No response";
+pingResponse.message = this.stringService.pingNoResponse;
-pingResponse.message = "Success";
+pingResponse.message = this.stringService.pingSuccess;Also applies to: 97-97
| @@ -257,12 +258,12 @@ class NetworkService { | |||
| if (error) { | |||
| dockerResponse.status = false; | |||
| dockerResponse.code = error.statusCode || this.NETWORK_ERROR; | |||
| dockerResponse.message = error.reason || errorMessages.DOCKER_FAIL; | |||
| dockerResponse.message = error.reason || "Failed to fetch Docker container information"; | |||
There was a problem hiding this comment.
Mom's spaghetti: Replace hardcoded messages in requestDocker! 🍝
The messages "Failed to fetch Docker container information" and "Docker container status fetched successfully" are hardcoded.
Apply this diff to use stringService:
-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.dockerFetchSuccess;Also applies to: 266-266
|
Hi @Cihatata! This is a great PR. I used I noticed the code uses both |
Describe your changes
This is a continuation of the closed Pull Request. => #1647
Add i18n Support & Fetch Translations in CI/CD
We decided not to store the POEditor API key in the repository. Instead, translations will now be fetched during the CI/CD pipeline and committed to the repository. On the server side, translations will be served directly from the locales directory.
To enable fetching translations from POEditor in the pipeline, we need a new GitHub Action. However, I couldn't add it due to insufficient permissions.
This PR introduces i18n support for Server while ensuring a secure and automated way to manage translations.
Issue number
#1396
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.