refactor: optimize nickname update logic in Discord actions #2487
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Unsafe Dynamic Type Mapping ▹ view | |
 |
Incorrect success condition for nickname updates ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| models/progresses.js | ✅ |
| controllers/discordactions.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| const { type, typeId, date } = pathParams; | ||
| const { dev } = queryParams; | ||
| /* eslint-disable security/detect-object-injection */ | ||
| await assertUserOrTaskExists({ [TYPE_MAP[type]]: typeId }); | ||
| const query = buildQueryToSearchProgressByDay({ [TYPE_MAP[type]]: typeId, date }); | ||
| /* eslint-enable security/detect-object-injection */ |
There was a problem hiding this comment.
Unsafe Dynamic Type Mapping 
Tell me more
What is the issue?
The code uses dynamic property access with TYPE_MAP[type] and disables security linting, indicating a potential design flaw in type handling.
Why this matters
Using dynamic property access and disabling security checks makes the code more vulnerable and harder to maintain. A more type-safe approach would improve code reliability and security.
Suggested change ∙ Feature Preview
const typeMappers = {
user: (typeId) => ({ userId: typeId }),
task: (typeId) => ({ taskId: typeId })
};
if (!typeMappers[type]) {
throw new Error('Invalid type');
}
const mappedParams = typeMappers[type](typeId);
await assertUserOrTaskExists(mappedParams);
const query = buildQueryToSearchProgressByDay({ ...mappedParams, date });Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if (response?.message) { | ||
| counter++; | ||
| totalNicknamesUpdated.count++; | ||
| nickNameUpdatedUsers.push(id); | ||
| } |
There was a problem hiding this comment.
Incorrect success condition for nickname updates 
Tell me more
What is the issue?
The condition response?.message only checks for the existence of a message property, not whether the nickname update was actually successful.
Why this matters
This could lead to counting failed operations as successful updates if the Discord API returns an error message in the response.message field, resulting in inaccurate reporting of update statistics.
Suggested change ∙ Feature Preview
Check for a proper success indicator instead of just the presence of a message. For example:
if (response?.success || (response?.message && !response?.error)) {
counter++;
totalNicknamesUpdated.count++;
nickNameUpdatedUsers.push(id);
}Or verify the actual structure of the setUserDiscordNickname response to determine the correct success condition.
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Date:
Developer Name:
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Refactor the Discord nickname update loop to use a for-of iteration, streamline validation and response handling, and improve how updated user IDs are tracked, plus add targeted ESLint directives around dynamic property access in progress queries.
Why are these changes being made?
Improve readability and reliability of nickname updates, reduce complexity in the loop, and ensure consistent handling of update results and errors. The added ESLint comments address a security rule while preserving existing behavior.