Improve party combat coordination and C4 character rendering - #76
Conversation
📝 WalkthroughWalkthroughThis change adds throttled bot party-chat coordination and authoritative skill-result confirmations, centralizes NPC aggro handling, updates companion combat and pulling behavior, improves movement visibility and packet state serialization, narrows equipment persistence writes, and adds regression tests. ChangesBot coordination and combat
Estimated code review effort: 5 (Critical) | ~90 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/GameServer/Actor/Generics/MoveTo.js (1)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize
CLIENT_VISIBILITY_RADIUSto avoid drift withUpdateEnvironment.js.
UpdateEnvironment.jshardcodes the same 6000 visibility radius (forfetchNpcsInRadius/fetchVisibleUsers) that this file now namesCLIENT_VISIBILITY_RADIUS. Both comments explicitly call out that they must stay in sync ("this same radius", "shared... gate"), which makes the duplication fragile — a future change to one won't visibly break anything until movement/aggro silently diverges from what's actually broadcast to clients.♻️ Suggested fix: single source of truth
+// src/GameServer/Constants/VisibilityRadius.js +module.exports = 6000;-const CLIENT_VISIBILITY_RADIUS = 6000; +const CLIENT_VISIBILITY_RADIUS = invoke('GameServer/Constants/VisibilityRadius');Also applies to: 26-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GameServer/Actor/Generics/MoveTo.js` around lines 5 - 8, Centralize the 6000 visibility radius used by MoveTo.js and UpdateEnvironment.js into one shared exported constant. Update MoveTo’s CLIENT_VISIBILITY_RADIUS and UpdateEnvironment’s fetchNpcsInRadius/fetchVisibleUsers checks to consume that single source of truth, preserving the existing visibility behavior.src/GameServer/Bot/AI/PartyPulling.js (1)
232-237: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
hasDeadPartyMemberscans all sessions on everycurrent()/tickBotPullercall.
current()is called multiple times per companion tick, and each call already runspauseReason. This helper adds a fullWorld.user.sessionswalk with anisPartySessionpredicate per member. Prefer reusingPartyAwareness.partySessions(leaderSession), which is already the party-scoped accessor used elsewhere in this module.♻️ Reuse the party-scoped accessor
function hasDeadPartyMember(leaderSession) { - return (World.user?.sessions || []).some((memberSession) => ( - PartyAwareness.isPartySession(memberSession, leaderSession) && - memberSession.actor?.isDead?.() === true - )); + return PartyAwareness.partySessions(leaderSession) + .some((memberSession) => memberSession.actor?.isDead?.() === true); }Verify that
partySessionsincludes dead members before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GameServer/Bot/AI/PartyPulling.js` around lines 232 - 237, Update hasDeadPartyMember to iterate over PartyAwareness.partySessions(leaderSession) instead of scanning World.user.sessions and calling isPartySession for each member. Preserve the dead-member check and verify that partySessions includes dead sessions before relying on it.src/GameServer/Bot/AI/States/FollowingState.js (3)
344-362: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRadius bump multiplies an already repeated scan.
partyAggroCountrunsWorld.fetchNpcsInRadiusonce per party actor, and the tick calls it up to three times (Lines 777, 868, 1021). Widening 900→1500 roughly triples the scanned area per call. Consider computingactiveMobsonce per tick and passing it down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GameServer/Bot/AI/States/FollowingState.js` around lines 344 - 362, Reduce repeated NPC scanning by computing the party’s active mobs once per tick and reusing that result across the up to three partyAggroCount call sites. Update partyAggroCount and its callers to accept and reuse the precomputed collection while preserving deduplication, attackability, death, and destination filtering.
1060-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated support-role and target-eligibility predicates.
['healer','buffer']is encoded three ways in this tick (supportCanMeleeAssist,basicAttackOnlyat Lines 1082 and 1130/1161), and theplayerTargetId && ... !== bot ... !== playercondition is repeated verbatim in Lines 1094 and 1099. Extracting aSUPPORT_ROLESset plus anassistableLeaderTargetlocal would keep these in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GameServer/Bot/AI/States/FollowingState.js` around lines 1060 - 1099, In the affected tick logic, centralize the healer/buffer classification in a shared SUPPORT_ROLES set and reuse it for supportCanMeleeAssist and each basicAttackOnly assignment. Also compute one assistableLeaderTarget value for the repeated playerTargetId eligibility check, then reuse it in both branches while preserving the existing exclusions for the bot and player IDs.
243-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemplate phrasing doesn't fit all
detailvariants.
detailis a noun phrase formarket_purchase("Longsword from X") but a verb phrase for the other two ("sell these resources to X","restock my shots"). Interpolating both intoQuick ${detail}/Taking a moment to ${detail}yields lines like "Quick sell these resources to X" and "Taking a moment to Longsword from X".✏️ Normalize detail to a verb phrase
const detail = errand.kind === 'market_purchase' - ? `${errand.itemName} from ${errand.target.name}` + ? `buy ${errand.itemName} from ${errand.target.name}` : errand.kind === 'sell_resources' ? `sell these resources to ${errand.target.name}` : 'restock my shots'; BotPartyChat.announce(session, { priority: 'informational', key: `town-errand:${bot.fetchId()}:${errand.kind}`, templates: [ - `Quick ${detail}; then I'm back to camp.`, + `Going to ${detail}; then I'm back to camp.`, `Taking a moment to ${detail}, then returning.` ] });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GameServer/Bot/AI/States/FollowingState.js` around lines 243 - 255, Update the errand detail construction in the surrounding announcement logic to normalize every variant into a verb phrase: make the market_purchase text describe the bot buying the item from errand.target.name, while preserving the existing sell_resources and restock wording. Keep both templates unchanged so all interpolated messages remain grammatically correct.tests/test_party_revival.js (1)
115-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the positive case: a close mob that is attacking must block resurrection.
Both new scenarios assert
handled === true. Nothing pins the behaviour the newfetchStateAttackcondition exists to preserve — a nearby hostile withfetchStateAttack: () => trueandfetchDestIdon the corpse should maketickreturnblockedBy: 'hostile_combat_record'. Without it, a regression that drops the corpse guard entirely still passes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_party_revival.js` around lines 115 - 132, Add a positive assertion in the PartyRevivalService.tick scenarios using the nearby spawn configured with fetchStateAttack: () => true and fetchDestId targeting the corpse. Verify the result is handled and has blockedBy equal to 'hostile_combat_record', preserving the corpse guard behavior before resetting leaderSession.partyRevivalAttempt.src/GameServer/Bot/AI/PartyRevivalService.js (1)
102-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate inline
invoke+expectSkillResultblocks.The scroll and skill paths repeat the same
invoke('GameServer/Bot/AI/BotPartyChat').expectSkillResult(...)call with only the target differing. A smallexpectResurrectionResult(session, target, skill)helper (with the module resolved once) would keep the two paths from drifting.Also applies to: 164-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GameServer/Bot/AI/PartyRevivalService.js` around lines 102 - 113, Extract the duplicated resurrection-result handling from castScroll and the corresponding skill path into an expectResurrectionResult(session, target, skill) helper. Resolve the BotPartyChat module once, then have both paths call the helper while preserving the existing target, skill, and kind: 'resurrection' arguments.src/GameServer/Bot/BotAI.js (1)
497-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist
BOW_ATTACK_RANGEto module scope.Weapon.Bowis the established weapon-kind string here, so there isn’t a shared constant to replace it with.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/GameServer/Bot/BotAI.js` around lines 497 - 498, Move the BOW_ATTACK_RANGE constant out of the local scope in the bot attack logic and define it at module scope in BotAI.js. Keep its value unchanged and continue using the existing Weapon.Bow string for hasBow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/GameServer/Bot/AI/BotPartyChat.js`:
- Around line 92-113: Update announce() so state.lastPartyMessageAt is advanced
only after a successful party-wide botPartySay, not after private botTell
confirmations. Preserve event timestamp tracking for both send types and keep
the existing send-success checks unchanged.
In `@src/GameServer/Bot/AI/States/FollowingState.js`:
- Around line 853-863: Update the rebuff announcement flow in FollowingState so
lastRebuffRequestAt is assigned only after BotPartyChat.announce confirms the
announcement was sent. Preserve the existing 90-second eligibility check and
announcement payload, and do not consume the cooldown when announce returns
false.
In `@src/GameServer/Network/Response/NpcInfo.js`:
- Line 6: Update the dead-state calculation in NpcInfo to detect whether
npc.state.fetchDead exists, not whether its result is truthy: use that
state-machine result, including explicit false, and only fall back to
fetchStateDead when the method is unavailable. Add the inverse regression case
covering state-machine false with legacy true.
In `@src/GameServer/Npc/NpcAggro.js`:
- Around line 63-78: Update startAggroTicker’s interval callback to catch errors
from tickLiveActors so a failure does not escape the timer and terminate the
process. Prefer isolating failures per session by handling exceptions around
engageNearby within tickLiveActors, allowing remaining live sessions to continue
processing each tick; preserve the existing ticker setup and return behavior.
---
Nitpick comments:
In `@src/GameServer/Actor/Generics/MoveTo.js`:
- Around line 5-8: Centralize the 6000 visibility radius used by MoveTo.js and
UpdateEnvironment.js into one shared exported constant. Update MoveTo’s
CLIENT_VISIBILITY_RADIUS and UpdateEnvironment’s
fetchNpcsInRadius/fetchVisibleUsers checks to consume that single source of
truth, preserving the existing visibility behavior.
In `@src/GameServer/Bot/AI/PartyPulling.js`:
- Around line 232-237: Update hasDeadPartyMember to iterate over
PartyAwareness.partySessions(leaderSession) instead of scanning
World.user.sessions and calling isPartySession for each member. Preserve the
dead-member check and verify that partySessions includes dead sessions before
relying on it.
In `@src/GameServer/Bot/AI/PartyRevivalService.js`:
- Around line 102-113: Extract the duplicated resurrection-result handling from
castScroll and the corresponding skill path into an
expectResurrectionResult(session, target, skill) helper. Resolve the
BotPartyChat module once, then have both paths call the helper while preserving
the existing target, skill, and kind: 'resurrection' arguments.
In `@src/GameServer/Bot/AI/States/FollowingState.js`:
- Around line 344-362: Reduce repeated NPC scanning by computing the party’s
active mobs once per tick and reusing that result across the up to three
partyAggroCount call sites. Update partyAggroCount and its callers to accept and
reuse the precomputed collection while preserving deduplication, attackability,
death, and destination filtering.
- Around line 1060-1099: In the affected tick logic, centralize the
healer/buffer classification in a shared SUPPORT_ROLES set and reuse it for
supportCanMeleeAssist and each basicAttackOnly assignment. Also compute one
assistableLeaderTarget value for the repeated playerTargetId eligibility check,
then reuse it in both branches while preserving the existing exclusions for the
bot and player IDs.
- Around line 243-255: Update the errand detail construction in the surrounding
announcement logic to normalize every variant into a verb phrase: make the
market_purchase text describe the bot buying the item from errand.target.name,
while preserving the existing sell_resources and restock wording. Keep both
templates unchanged so all interpolated messages remain grammatically correct.
In `@src/GameServer/Bot/BotAI.js`:
- Around line 497-498: Move the BOW_ATTACK_RANGE constant out of the local scope
in the bot attack logic and define it at module scope in BotAI.js. Keep its
value unchanged and continue using the existing Weapon.Bow string for hasBow.
In `@tests/test_party_revival.js`:
- Around line 115-132: Add a positive assertion in the PartyRevivalService.tick
scenarios using the nearby spawn configured with fetchStateAttack: () => true
and fetchDestId targeting the corpse. Verify the result is handled and has
blockedBy equal to 'hostile_combat_record', preserving the corpse guard behavior
before resetting leaderSession.partyRevivalAttempt.
🪄 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 Plus
Run ID: 2cb95d07-fe54-4310-8d56-b4e99cb83890
📒 Files selected for processing (40)
scripts/run-tests.jssrc/GameServer/Actor/Attack.jssrc/GameServer/Actor/Backpack.jssrc/GameServer/Actor/Generics/MoveTo.jssrc/GameServer/Actor/Generics/StopAutomation.jssrc/GameServer/Actor/Generics/UpdateEnvironment.jssrc/GameServer/Automation.jssrc/GameServer/Bot/AI/BotCombatUtility.jssrc/GameServer/Bot/AI/BotPartyChat.jssrc/GameServer/Bot/AI/BotRoles.jssrc/GameServer/Bot/AI/PartyAwareness.jssrc/GameServer/Bot/AI/PartyCombatState.jssrc/GameServer/Bot/AI/PartyCompanionService.jssrc/GameServer/Bot/AI/PartyPulling.jssrc/GameServer/Bot/AI/PartyRevivalService.jssrc/GameServer/Bot/AI/States/FollowingState.jssrc/GameServer/Bot/BotAI.jssrc/GameServer/Bot/BotManager.jssrc/GameServer/Effects/EffectRestrictions.jssrc/GameServer/Network/Response/CharInfo.jssrc/GameServer/Network/Response/NpcInfo.jssrc/GameServer/Network/Response/UserInfo.jssrc/GameServer/Npc/Npc.jssrc/GameServer/Npc/NpcAggro.jssrc/GameServer/World/Generics/NpcBypasses/CompanionControl.jssrc/GameServer/World/Generics/SpawnNpcs.jssrc/GameServer/World/World.jstests/test_bot_chat_commands.jstests/test_bot_combat_skill_selection.jstests/test_bot_movement_visibility.jstests/test_bot_party_chat.jstests/test_c4_protocol_packets.jstests/test_equipment_slots.jstests/test_npc_combat_range.jstests/test_npc_hot_bot_aggro.jstests/test_party_bot_loot.jstests/test_party_companion_rest_follow.jstests/test_party_pull_pause.jstests/test_party_revival.jstests/test_trade_equipment_upgrade.js
| function announce(session, entry = {}) { | ||
| if (!session?.actor || !entry.key) return false; | ||
| // A factual reply to a direct player request also matters outside a | ||
| // companion party. Ambient and coordination events remain party-only. | ||
| const state = stateFor(session, !!entry.targetSession); | ||
| if (!state) return false; | ||
|
|
||
| const now = Number(entry.now || Date.now()); | ||
| if (!canSend(state, entry, now)) return false; | ||
| const text = chooseText(entry, state); | ||
| if (!text) return false; | ||
|
|
||
| const BotManager = invoke('GameServer/Bot/BotManager'); | ||
| const sent = entry.targetSession | ||
| ? BotManager.botTell(session, entry.targetSession, text) | ||
| : BotManager.botPartySay(session, text); | ||
| if (sent === false) return false; | ||
|
|
||
| state.events[entry.key] = now; | ||
| state.lastPartyMessageAt = now; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Private tells pollute the shared party-broadcast cooldown.
announce() updates state.lastPartyMessageAt for every successful send, including private botTell confirmations (heal/buff results). Since stateFor() always resolves to the shared leader-level state for a party companion, a one-on-one heal/buff confirmation that the rest of the party never sees will still reset the party's broadcast cooldown clock — silently suppressing the next genuine party-wide coordination/informational callout (add warnings, pull announcements, etc.) for up to 7–15 seconds.
🐛 Proposed fix
state.events[entry.key] = now;
- state.lastPartyMessageAt = now;
+ // A private tell is invisible to the rest of the party and must not
+ // consume the shared party-broadcast cooldown budget.
+ if (!entry.targetSession) {
+ state.lastPartyMessageAt = now;
+ }
return true;📝 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.
| function announce(session, entry = {}) { | |
| if (!session?.actor || !entry.key) return false; | |
| // A factual reply to a direct player request also matters outside a | |
| // companion party. Ambient and coordination events remain party-only. | |
| const state = stateFor(session, !!entry.targetSession); | |
| if (!state) return false; | |
| const now = Number(entry.now || Date.now()); | |
| if (!canSend(state, entry, now)) return false; | |
| const text = chooseText(entry, state); | |
| if (!text) return false; | |
| const BotManager = invoke('GameServer/Bot/BotManager'); | |
| const sent = entry.targetSession | |
| ? BotManager.botTell(session, entry.targetSession, text) | |
| : BotManager.botPartySay(session, text); | |
| if (sent === false) return false; | |
| state.events[entry.key] = now; | |
| state.lastPartyMessageAt = now; | |
| return true; | |
| } | |
| function announce(session, entry = {}) { | |
| if (!session?.actor || !entry.key) return false; | |
| // A factual reply to a direct player request also matters outside a | |
| // companion party. Ambient and coordination events remain party-only. | |
| const state = stateFor(session, !!entry.targetSession); | |
| if (!state) return false; | |
| const now = Number(entry.now || Date.now()); | |
| if (!canSend(state, entry, now)) return false; | |
| const text = chooseText(entry, state); | |
| if (!text) return false; | |
| const BotManager = invoke('GameServer/Bot/BotManager'); | |
| const sent = entry.targetSession | |
| ? BotManager.botTell(session, entry.targetSession, text) | |
| : BotManager.botPartySay(session, text); | |
| if (sent === false) return false; | |
| state.events[entry.key] = now; | |
| // A private tell is invisible to the rest of the party and must not | |
| // consume the shared party-broadcast cooldown budget. | |
| if (!entry.targetSession) { | |
| state.lastPartyMessageAt = now; | |
| } | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/GameServer/Bot/AI/BotPartyChat.js` around lines 92 - 113, Update
announce() so state.lastPartyMessageAt is advanced only after a successful
party-wide botPartySay, not after private botTell confirmations. Preserve event
timestamp tracking for both send types and keep the existing send-success checks
unchanged.
| if (rebuff && rebuff.provider !== bot && Date.now() - (session.lastRebuffRequestAt || 0) > 90000) { | ||
| session.lastRebuffRequestAt = Date.now(); | ||
| BotAI.say(session, `${rebuff.provider.fetchName()}, could you refresh ${rebuff.skill.fetchName()}?`); | ||
| BotPartyChat.announce(session, { | ||
| priority: 'coordination', | ||
| key: `rebuff:${rebuff.provider.fetchId()}:${rebuff.skill.fetchSelfId()}`, | ||
| templates: [ | ||
| `${rebuff.provider.fetchName()}, refresh ${rebuff.skill.fetchName()} when safe?`, | ||
| `${rebuff.provider.fetchName()}, ${rebuff.skill.fetchName()} is fading.` | ||
| ] | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
lastRebuffRequestAt is consumed even when the announcement is dropped.
BotPartyChat.announce can return false (throttle/dedupe/no party). Setting the timestamp beforehand silences the rebuff request for another 90s without anyone hearing it.
♻️ Only record the request when it was actually sent
- session.lastRebuffRequestAt = Date.now();
- BotPartyChat.announce(session, {
+ const requested = BotPartyChat.announce(session, {
priority: 'coordination',
key: `rebuff:${rebuff.provider.fetchId()}:${rebuff.skill.fetchSelfId()}`,
templates: [
`${rebuff.provider.fetchName()}, refresh ${rebuff.skill.fetchName()} when safe?`,
`${rebuff.provider.fetchName()}, ${rebuff.skill.fetchName()} is fading.`
]
- });
+ });
+ if (requested) session.lastRebuffRequestAt = Date.now();📝 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.
| if (rebuff && rebuff.provider !== bot && Date.now() - (session.lastRebuffRequestAt || 0) > 90000) { | |
| session.lastRebuffRequestAt = Date.now(); | |
| BotAI.say(session, `${rebuff.provider.fetchName()}, could you refresh ${rebuff.skill.fetchName()}?`); | |
| BotPartyChat.announce(session, { | |
| priority: 'coordination', | |
| key: `rebuff:${rebuff.provider.fetchId()}:${rebuff.skill.fetchSelfId()}`, | |
| templates: [ | |
| `${rebuff.provider.fetchName()}, refresh ${rebuff.skill.fetchName()} when safe?`, | |
| `${rebuff.provider.fetchName()}, ${rebuff.skill.fetchName()} is fading.` | |
| ] | |
| }); | |
| } | |
| if (rebuff && rebuff.provider !== bot && Date.now() - (session.lastRebuffRequestAt || 0) > 90000) { | |
| const requested = BotPartyChat.announce(session, { | |
| priority: 'coordination', | |
| key: `rebuff:${rebuff.provider.fetchId()}:${rebuff.skill.fetchSelfId()}`, | |
| templates: [ | |
| `${rebuff.provider.fetchName()}, refresh ${rebuff.skill.fetchName()} when safe?`, | |
| `${rebuff.provider.fetchName()}, ${rebuff.skill.fetchName()} is fading.` | |
| ] | |
| }); | |
| if (requested) session.lastRebuffRequestAt = Date.now(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/GameServer/Bot/AI/States/FollowingState.js` around lines 853 - 863,
Update the rebuff announcement flow in FollowingState so lastRebuffRequestAt is
assigned only after BotPartyChat.announce confirms the announcement was sent.
Preserve the existing 90-second eligibility check and announcement payload, and
do not consume the cooldown when announce returns false.
|
|
||
| function npcInfo(npc) { | ||
| const packet = new SendPacket(0x16); | ||
| const deadState = npc.state?.fetchDead?.() ? 0x01 : (npc.fetchStateDead() ? 0x01 : 0x00); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor an explicit state-machine false.
When npc.state.fetchDead() returns false, this falls through to fetchStateDead(). A stale legacy true can therefore render a revived NPC as dead. Branch on whether fetchDead exists, rather than its truthiness, and add the inverse regression case.
Proposed fix
- const deadState = npc.state?.fetchDead?.() ? 0x01 : (npc.fetchStateDead() ? 0x01 : 0x00);
+ const deadState = typeof npc.state?.fetchDead === 'function'
+ ? (npc.state.fetchDead() ? 0x01 : 0x00)
+ : (npc.fetchStateDead() ? 0x01 : 0x00);📝 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 deadState = npc.state?.fetchDead?.() ? 0x01 : (npc.fetchStateDead() ? 0x01 : 0x00); | |
| const deadState = typeof npc.state?.fetchDead === 'function' | |
| ? (npc.state.fetchDead() ? 0x01 : 0x00) | |
| : (npc.fetchStateDead() ? 0x01 : 0x00); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/GameServer/Network/Response/NpcInfo.js` at line 6, Update the dead-state
calculation in NpcInfo to detect whether npc.state.fetchDead exists, not whether
its result is truthy: use that state-machine result, including explicit false,
and only fall back to fetchStateDead when the method is unavailable. Add the
inverse regression case covering state-machine false with legacy true.
| function tickLiveActors(world = invoke('GameServer/World/World'), now = Date.now()) { | ||
| return (world?.user?.sessions || []) | ||
| .filter(isLiveSession) | ||
| .flatMap((session) => engageNearby(session, session.actor, { world, now })); | ||
| } | ||
|
|
||
| function startAggroTicker(world = invoke('GameServer/World/World'), { | ||
| setTicker = setInterval | ||
| } = {}) { | ||
| if (!world || world.npcAggroTicker) return world?.npcAggroTicker; | ||
|
|
||
| const ticker = setTicker(() => tickLiveActors(world), 1000); | ||
| ticker?.unref?.(); | ||
| world.npcAggroTicker = ticker; | ||
| return ticker; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the per-second aggro ticker against thrown errors.
startAggroTicker runs tickLiveActors in a bare setInterval for the lifetime of the server. Any exception inside engageNearby/enterCombatState for a single malformed session or NPC (e.g. missing dataSendToMeAndOthers, stale actor state) will throw uncaught from the timer callback, which can crash the whole process every second going forward — this sweeps all live sessions unconditionally.
🛡️ Proposed fix: isolate per-session failures
function tickLiveActors(world = invoke('GameServer/World/World'), now = Date.now()) {
return (world?.user?.sessions || [])
.filter(isLiveSession)
- .flatMap((session) => engageNearby(session, session.actor, { world, now }));
+ .flatMap((session) => {
+ try {
+ return engageNearby(session, session.actor, { world, now });
+ } catch (err) {
+ console.error('[NpcAggro] tick failed for session', session?.accountId, err);
+ return [];
+ }
+ });
}📝 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.
| function tickLiveActors(world = invoke('GameServer/World/World'), now = Date.now()) { | |
| return (world?.user?.sessions || []) | |
| .filter(isLiveSession) | |
| .flatMap((session) => engageNearby(session, session.actor, { world, now })); | |
| } | |
| function startAggroTicker(world = invoke('GameServer/World/World'), { | |
| setTicker = setInterval | |
| } = {}) { | |
| if (!world || world.npcAggroTicker) return world?.npcAggroTicker; | |
| const ticker = setTicker(() => tickLiveActors(world), 1000); | |
| ticker?.unref?.(); | |
| world.npcAggroTicker = ticker; | |
| return ticker; | |
| } | |
| function tickLiveActors(world = invoke('GameServer/World/World'), now = Date.now()) { | |
| return (world?.user?.sessions || []) | |
| .filter(isLiveSession) | |
| .flatMap((session) => { | |
| try { | |
| return engageNearby(session, session.actor, { world, now }); | |
| } catch (err) { | |
| console.error('[NpcAggro] tick failed for session', session?.accountId, err); | |
| return []; | |
| } | |
| }); | |
| } | |
| function startAggroTicker(world = invoke('GameServer/World/World'), { | |
| setTicker = setInterval | |
| } = {}) { | |
| if (!world || world.npcAggroTicker) return world?.npcAggroTicker; | |
| const ticker = setTicker(() => tickLiveActors(world), 1000); | |
| ticker?.unref?.(); | |
| world.npcAggroTicker = ticker; | |
| return ticker; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 73-73: React's useState should not be directly called
Context: setTicker(() => tickLiveActors(world), 1000)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/GameServer/Npc/NpcAggro.js` around lines 63 - 78, Update
startAggroTicker’s interval callback to catch errors from tickLiveActors so a
failure does not escape the timer and terminate the process. Prefer isolating
failures per session by handling exceptions around engageNearby within
tickLiveActors, allowing remaining live sessions to continue processing each
tick; preserve the existing ticker setup and return behavior.
1 participant
Summary
This PR improves companion-party combat reliability and corrects several C4 client presentation paths.
CharInfoandUserInfo: the field after position is the boat object id, not heading. Sending heading there made normal characters look attached to a non-existent vehicle and could corrupt their client presentation.Player impact
Parties should stay together while a tank pulls, engage delivered mobs without an idle delay, defend against distant ranged adds, and recover from deaths more reliably. Support companions behave like support characters instead of joining melee with caster weapons. Bot movement and character rendering should look more stable to nearby players.
Root cause
The C4 character packets used the actor heading in the boat-object slot. The C4 protocol source expects
0for a character that is not on a boat. This affected every visible player character, including idle merchants, rather than only bots in combat.Validation
npm testnode tests/test_c4_protocol_packets.jsnode scripts/check-syntax.jsgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes