refactor: remove redundant code in NPC helpers and event sorting#710
Open
kamilwronka wants to merge 1 commit intodevelopfrom
Open
refactor: remove redundant code in NPC helpers and event sorting#710kamilwronka wants to merge 1 commit intodevelopfrom
kamilwronka wants to merge 1 commit intodevelopfrom
Conversation
- Remove redundant property overrides in composeNpcFromGame where nick, prof, wt, lvl, type were re-assigned from npc after already being spread via ...npc - Remove unnecessary optional chaining on getTime method call in find-active-event-heroes-by-npc.ts (createdAt?.getTime?.() to createdAt?.getTime() — if createdAt exists, getTime is guaranteed) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughFive separate refactoring and cleanup changes across chat, Discord, event, guild, and game client services. Includes removing an await statement, extracting duplicated error-checking logic, fixing optional-chaining syntax, consolidating guild-lookup methods, and removing unused field copies from NPC composition. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying lootlog-landing with
|
| Latest commit: |
2f998cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1093ba6a.lootlog-landing.pages.dev |
| Branch Preview URL: | https://refactor-small-cleanup-redun.lootlog-landing.pages.dev |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
lootlog-docs | 2f998cb | Commit Preview URL Branch Preview URL |
Apr 08 2026, 05:13 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/guilds/guilds.service.ts`:
- Around line 107-112: The current check uses allGuildsInDiscord =
guilds.every(g => discordGuildIds.includes(g.id)) which is always true because
guilds were fetched with id: { in: discordGuildIds }; instead change the
predicate to verify that every id in discordGuildIds exists in the fetched
guilds (e.g., discordGuildIds.every(id => guilds.some(g => g.id === id)) or
compare sets/lengths) before deciding to call
this.discordService.clearUserGuildIdsCache(userId); update both occurrences (the
one using allGuildsInDiscord and the call to clearUserGuildIdsCache) so the
cache is invalidated when there are missing guild IDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dfb7333-488d-4aba-9451-111eef24c23c
📒 Files selected for processing (5)
apps/api/src/chat/chat.service.tsapps/api/src/discord/discord.service.tsapps/api/src/events/utils/find-active-event-heroes-by-npc.tsapps/api/src/guilds/guilds.service.tsapps/game-client/src/hooks/game-events/helpers/npc.helpers.ts
💤 Files with no reviewable changes (1)
- apps/game-client/src/hooks/game-events/helpers/npc.helpers.ts
Comment on lines
+107
to
112
| const allGuildsInDiscord = guilds.every((guild) => { | ||
| return discordGuildIds.includes(guild.id); | ||
| }); | ||
|
|
||
| if (!comparedGuilds) { | ||
| if (!allGuildsInDiscord) { | ||
| await this.discordService.clearUserGuildIdsCache(userId); |
There was a problem hiding this comment.
Cache invalidation condition is effectively always true and never triggers.
At Line 107 and Line 501, allGuildsInDiscord is derived from guilds.every((guild) => discordGuildIds.includes(guild.id)), but guilds was already fetched with id: { in: discordGuildIds }. That makes the predicate trivially true, so Lines 111/505 won’t clear stale cache entries.
Suggested fix (apply in both locations)
-const allGuildsInDiscord = guilds.every((guild) => {
- return discordGuildIds.includes(guild.id);
-});
-
-if (!allGuildsInDiscord) {
+const dbGuildIds = new Set(guilds.map((guild) => guild.id));
+const hasGuildIdsMissingInDb = discordGuildIds.some(
+ (discordGuildId) => !dbGuildIds.has(discordGuildId),
+);
+
+if (hasGuildIdsMissingInDb) {
await this.discordService.clearUserGuildIdsCache(userId);
}Also applies to: 501-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/guilds/guilds.service.ts` around lines 107 - 112, The current
check uses allGuildsInDiscord = guilds.every(g =>
discordGuildIds.includes(g.id)) which is always true because guilds were fetched
with id: { in: discordGuildIds }; instead change the predicate to verify that
every id in discordGuildIds exists in the fetched guilds (e.g.,
discordGuildIds.every(id => guilds.some(g => g.id === id)) or compare
sets/lengths) before deciding to call
this.discordService.clearUserGuildIdsCache(userId); update both occurrences (the
one using allGuildsInDiscord and the call to clearUserGuildIdsCache) so the
cache is invalidated when there are missing guild IDs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
composeNpcFromGame— the spread...npcalready includesnick,prof,wt,lvl,typefromGameNpc, so re-assigning them with identical values was dead codegetTimemethod infind-active-event-heroes-by-npc.ts— changedcreatedAt?.getTime?.()tocreatedAt?.getTime()since ifcreatedAtis a Date,getTimeis guaranteed to existSafety
Files touched
apps/game-client/src/hooks/game-events/helpers/npc.helpers.tsapps/api/src/events/utils/find-active-event-heroes-by-npc.tsResidual risks
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor