Gameplay correctness: quest/state/economy fixes, combat+boss wiring, client truth pass, and a working Fracture Rift#2
Merged
Conversation
…ST_ABANDON formats
…n damage multipliers Finding 1 (nemesis dead end-to-end): handleHurt called world.handleHurtEntity(player) with no attacker, so player:died carried killerId:undefined and nemesis.service never fired. Now passes (player, mob, dmg) — the mob reaches handlePlayerDeath and the event carries killerId/killerType. Also applies mob.nemesisPowerLevel (set by nemesis.service but never consumed) as a mob->player damage multiplier. Finding 6 (ascension): getAscensionDamageMultiplier had zero callers; handleHit now scales player->mob damage by it (+5% per ascension, as PROGRESSION_INIT advertises).
…istribution distributePartyRewards fixes from the gameplay-correctness scan: - Finding 1c: wire nemesisService.getRevengeMultipliers (zero callers) in when the dead mob was the attacker's nemesis (2.5x XP / 2x gold). - Finding 4: consume RoamingBoss.getLootMultipliers (zero callers) so zone bosses pay with their config xp/gold multipliers. - Finding 5: apply the mob's-zone xpBonus/goldBonus via the ZoneManager singleton (applyXpBonus/applyGoldBonus had zero callers while ZONE_INFO advertises 5-50% bonuses). Uses the singleton because the CombatSystem world-context literal (world.ts) does not carry zoneManager; zone is looked up at the MOB position. - Finding 7: pass the attacker's real level to Formulas.xpFromMob so xpLevelModifier no longer compares against level 1 (level-40 players farming level-5 mobs got a bonus instead of ~0). Party shares use each member's own level. - Finding 9: call player.updatePartyHp?.() in handleHurtEntity so party HP bars track damage.
- Finding 2: spawnBoss never called boss.onMove(...) (unlike
mobarea.ts / spawn-manager.ts), so clients saw zone bosses frozen
while the server roamed them. Now wired to
world.onMobMoveCallback; BossWorld interface gains the method.
- Finding 3: world.getDroppedItem gates legendary drops on mob.bossId,
which RoamingBoss never set — zone-boss legendaries were impossible.
Constructor now sets this.bossId = config.id.
- Finding 4: bosses fought and paid as their base sprite kind while
getBaseDamage/getArmor sat unused. Constructor now overrides
weaponLevel/armorLevel from config and level from the zone maxLevel.
- Replace lazy require('./message') with a top-level import (no import
cycle; message.ts only depends on gametypes/items) so the module is
loadable under the ESM test runner.
Finding 6: getAscensionHpMultiplier (progression.handler) had zero callers, so PROGRESSION_INIT's advertised +5% max HP per ascension was never applied. All max-HP recalcs (equipment-driven player update, mob spawn) route through Character.resetHitPoints, so the multiplier is applied there; mobs carry no ascensionCount and are unaffected.
Finding 8: getGroupIdFromPosition used floor() group dims, so on the
172x314 map (6x26 full groups) tiles x in {169..171} and y in {312,313}
resolved to nonexistent groups — addToGroup skipped those entities and
removeFromGroups nulled entity.group (invisible players at the east
beach edge, ~2.3% ghost zone-boss spawns). gx/gy are now clamped into
[0, groupWidth-1]/[0, groupHeight-1]; a full-map sweep test asserts
every tile resolves to a group forEachGroup creates.
…static fallbacks (finding 2)
… canonical (finding 10)
…s the first (finding 11); also wires sendQuestAbandon dep (finding 7)
- Send Messages.QuestStatus after quest offer (progress 0) and after every non-completing kill/area progress tick, in both Venice and no-AI branches (finding 1: nothing sent QuestStatus). - On quest completion, grant quest.xp via player.grantXP and add the reward item to inventory, dropping at the player's feet when full (finding 2: rewards were never granted). - Route QUEST_ABANDON (105) to the active quest service and confirm with QuestStatus(null) so the client clears server-side (finding 3).
- Respawn (second HELLO on the same connection while dead) no longer reloads from storage, which clobbered up to 60s of progress with the last autosave. Keeps in-memory state, restores full HP, respawns at the last checkpoint, and resends WELCOME + EQUIP from live state (finding 4). First-login behavior is unchanged. - AchievementService.initPlayer is now idempotent: when the persistence layer already loaded saved state during login, the later init from auth.handler no longer overwrites it with a blank state (finding 5).
- ProgressionService.levelUp now invokes a checkSkillUnlocks callback, wired in Player to SkillHandler.checkSkillUnlock, so SKILL_UNLOCK reaches the client mid-session (finding 7: checkSkillUnlocks had no callers). - Player.grantXP writes the rested XP burn back from the progression context copy, so the rested bonus actually depletes (finding 8).
- EconomyService classifies consumables with Types.isExpendableItem, so CAKE and FIREPOTION take the inventory path instead of charging no gold and granting no item while stock still decremented (finding 9). - handleShopBuy checks inventory room BEFORE processPurchase so limited stock is no longer decremented when the buy then aborts on a full inventory (finding 10).
- handleUnequipToInventory now carries the equipped item's properties into inventory (and into INVENTORY_ADD) instead of null (finding 11). - The owner is sent the equip change directly after unequip — the broadcast ignores self (finding 12, mirrors the equip path). - Dropping part of a stack sends INVENTORY_UPDATE with the remaining count instead of INVENTORY_REMOVE while the server keeps the remainder (finding 13).
- AIPlayer.respawn re-registers the bot with the entity registry and players map — die() removed it, so bots respawned as invisible ghosts (finding 6). - MOVE handler calls player.updatePartyPosition after a successful move so party XP range checks and the party UI use live positions (finding 14: updatePartyPosition had no callers). - World regen tick calls player.updatePartyHp after healing so party HP bars don't freeze between damage events (finding 15). - RoamingBoss mobs (config present) are exempt from spawn-anchor leash resets in both the aggro tick and the move callback — they roam beyond leash range by design and rubber-banded mid-fight (finding 16).
Covers saveRiftEntry upsert, getRiftLeaderboard sort/limit, and uninitialized-db guards for both functions. The wiring on the rift-manager side (setLeaderboardSink + loadLeaderboard on boot) was already landed on this branch; these tests lock the storage layer that backs it.
The namesake feature was unreachable and unwired at every layer (scan finding D2). Closes the full gap list: - format.ts: register RIFT_ENTER/RIFT_EXIT/RIFT_LEADERBOARD_REQ (the checker used to disconnect on these) - Entry: status-bar rift button + R key + ESC (RIFT_PORTAL has no client sprite, so the menu-button path); in-rift exit via menu/ESC - Protocol: Messages.RiftStart/Progress/Advance/End/Leaderboard with positional serialize() matching the existing client parsers - Spawning: world 1s rift tick -> riftManager.getMobToSpawn (3s cadence, max 5 alive, map-validated positions) -> addMob -> registerSpawnedMob - Kills/death/disconnect: handleRiftKill/handleRiftDeath from combat-system, handleRiftDisconnect from world onExit (despawns leftover mobs) - Teleport: save position on enter, door-path teleport to the arena, restore on exit/death - Leaderboard: seeded from SQLite at boot + persisted per ranked run (memory-only fallback if storage unavailable) - Modifiers: FORTIFIED/EMPOWERED applied at spawn; the rest documented display-only (minimal modifier system) Built via delegated MiniMax-M3 session; verified by hand (full diff review + suite). Tests 70f/3317 green (+26), tsc + webpack clean.
- player.ts/world.ts: handleRiftDisconnect now restores the rift entry position and runs BEFORE saveToStorage — a disconnect inside the rift used to persist the arena position (codex) - combat-system.ts + rift-manager.ts: freeSpawnSlotForMob frees the owning run's spawn slot when a rift mob dies to a non-owner (party member / another player) — off-owner kills no longer stall the run at the spawn cap; no progress credit (codex) - world.ts: rift mob ids are '6' + zero-padded session counter — entity.ts parses ids with parseInt, so kind-encoding or separators break/collide (qwen, amplified: the kind+counter string concatenation was ambiguous and the separator variant parsed to a constant) - qwen leaderboard-handler finding: FALSE POSITIVE — handler exists at message-router.ts:388 (pre-existing; the PR only added format+client) - qwen equip() double-call finding: harmless — equip() is a pure message builder (player.ts 'return new Messages.EquipItem(this, item)') Tests: +4 (position restore, no-op disconnect, freeSpawnSlot ownership + unknown mob). Suite 70f/3321 green, tsc clean.
- progression.service: level-up skips the HP refill and HitPoints packet when the player is dead (rift death-reward level-up would resurrect the corpse's HP bar and confuse hitPoints<=0 cleanup) (codex) - client ESC: closes open rift panels (menu/leaderboard) before falling through to exitRift — closing an overlay no longer ends the run (codex) - minimap: returning players get auto-show too, matching the old default (qwen P3) - qwen unknown-reward-kind finding: SILENCED by design — an unresolvable kind string cannot be materialized or dropped; XP always lands, all 16 shipped templates have valid kinds (pinned by quest tests), the warn log is the audit trail
Reviewer's GuideServer and client gameplay correctness pass wiring previously standalone systems (quests, progression, combat, bosses, rift) to actual state, messages, and storage: Fracture Rift is now fully enterable, progresses and rewards correctly with positional protocol, quest progress and rewards are granted and abandon is handled, combat/XP/economy systems respect nemesis, zones, ascension, party, and boss configs, and several persistence/AI/world bugs are fixed. Sequence diagram for quest progress, completion, and reward deliverysequenceDiagram
actor P as Player
participant GC as GameClient
participant VH as VeniceHandler
participant QS as QuestServiceLike
participant Inv as Inventory
participant W as World
P->>GC: kill mob / enter area
GC->>VH: handleKill / handleAreaChange
VH->>QS: checkQuestProgress(playerId, type, target)
alt quest completed
QS-->>VH: QuestResult(completed=true, xp, reward)
VH->>GC: Messages.QuestComplete
VH->>VH: grantQuestRewards(ctx, result)
opt XP reward
VH->>P: grantXP(result.xp)
end
VH->>Inv: getInventory()
alt inventory hasRoom(kind)
VH->>Inv: addItem(kind, properties, 1)
VH->>GC: INVENTORY_ADD(slotIndex, kind, properties, count)
else inventory full or no inventory
VH->>W: createItemWithProperties(kind, ctx.x, ctx.y, null)
W-->>VH: SpawnableItem
VH->>W: addItem(item)
VH->>W: broadcast(Messages.Spawn)
end
else quest not yet complete
QS-->>VH: null
VH->>VH: sendQuestProgress(ctx, quests, type, target)
VH->>GC: Messages.QuestStatus(quest)
end
note over P,QS: QUEST_ABANDON
P->>GC: QUEST_ABANDON
GC->>VH: handleAbandonQuest
VH->>QS: abandonQuest(playerId)
VH->>GC: Messages.QuestStatus(null)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds Fracture Rift gameplay, client controls, server messaging, spawning, cleanup, and leaderboard persistence. It also synchronizes quest progress and abandonment, replaces retired AI presentation, and updates combat, progression, economy, inventory, map, and world behavior with regression coverage. ChangesFracture Rift
Quest synchronization and AI retirement
Gameplay systems and regressions
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant Client
participant GameServer
participant RiftManager
participant Storage
Player->>Client: open Rift menu
Client->>GameServer: enter Rift
GameServer->>RiftManager: start run and select spawn
RiftManager-->>GameServer: run state and mob spawn data
GameServer-->>Client: RiftStart or RiftProgress
Player->>GameServer: exit or die
GameServer->>RiftManager: end run
RiftManager->>Storage: persist leaderboard entry
GameServer-->>Client: RiftEnd and cleanup state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 security issues
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
this.menuElement.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="client/ts/ui/rift-ui.ts" line_range="193" />
<code_context>
this.menuElement.innerHTML = html;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 2
<location path="client/ts/ui/rift-ui.ts" line_range="193" />
<code_context>
this.menuElement.innerHTML = html;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `this.menuElement.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| html += `<button id="rift-menu-leaderboard" style="${buttonStyle}">Leaderboard</button>`; | ||
| html += `<button id="rift-menu-close" style="${buttonStyle} background: #444;">Close</button>`; | ||
|
|
||
| this.menuElement.innerHTML = html; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| html += `<button id="rift-menu-leaderboard" style="${buttonStyle}">Leaderboard</button>`; | ||
| html += `<button id="rift-menu-close" style="${buttonStyle} background: #444;">Close</button>`; | ||
|
|
||
| this.menuElement.innerHTML = html; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a this.menuElement.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
…eedback - venice.handler/quest.service: QuestResult carries the completed quest; a final QuestStatus (N/N) is sent before QuestComplete in all four branches — the tracker no longer jumps from a partial count to the completion toast (qwen) - client ESC: an active rift only exits when ESC closed nothing — any open window/panel (achievements, help, credits, rift panels) makes ESC a close gesture, not a run-ender (codex) - client rift-ui.handler: failed entries and other non-death failures now show the server's reason (was silent) (codex P3) - codex multi-world rift-mob id collision: dormant — nb_worlds=1 in production; noted with the existing boss-id-collision caveat in the scan doc
Owner
Author
|
Panel round 3 (review-223559) dispositions — FIXED in 62db492: codex ESC-exits-rift-with-window-open (ESC now only exits when it closed nothing); codex silent rift-entry failure (client shows the reason); qwen quest completion final progress sync (QuestResult carries the quest; final N/N QuestStatus precedes QuestComplete in all four branches, test updated). SILENCED: codex multi-world rift-mob id collision — dormant (nb_worlds=1 in production; noted alongside the existing boss-id-collision caveat). No fourth round: findings from the disposition run itself were micro-class and are all resolved above. Suite 70f/3321 green, tsc + webpack clean. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/ts/handlers/game-event.handler.ts">
<violation number="1" location="client/ts/handlers/game-event.handler.ts:127">
P3: showNotification is called twice back-to-back for title then description; since the notification UI appears to be a single-slot display, the title message risks being clobbered before it's visible. Consider staggering the description with a short setTimeout (similar to the warning in handleZoneEnter) or combining title+description into one notification.</violation>
</file>
<file name="client/ts/player/interaction-controller.ts">
<violation number="1" location="client/ts/player/interaction-controller.ts:220">
P2: Keying pending NPC talk purely by `npc.kind` (not by NPC instance) means talking to two NPCs of the same kind in quick succession (e.g. the map's 10 'guard' NPCs) can cross-attribute the AI response to the wrong NPC, and the first NPC's '...' bubble never resolves since its fallback-timeout equality check fails. Consider keying by a unique NPC id (or kind+id) instead of kind alone.</violation>
</file>
<file name="server/ts/player/shop.handler.ts">
<violation number="1" location="server/ts/player/shop.handler.ts:52">
P3: Now that inventory room is verified upfront before processPurchase, the duplicate hasRoom checks inside the isWeapon/isArmor and isConsumable branches further down are unreachable in the normal synchronous flow and could be removed for clarity.</violation>
</file>
<file name="server/ts/rifts/rift-manager.ts">
<violation number="1" location="server/ts/rifts/rift-manager.ts:341">
P3: `freeSpawnSlotForMob` does a linear scan across all active rift runs for every kill in the game (it's invoked unconditionally from combat-system.ts, not gated on `isRiftMob`). Consider maintaining a `mobId -> playerId` index in the manager so this lookup is O(1) instead of O(active runs) per kill.</violation>
</file>
<file name="server/ts/player/auth.handler.ts">
<violation number="1" location="server/ts/player/auth.handler.ts:42">
P2: If `world.addPlayer(player)` fails on respawn, the client still receives WELCOME/EQUIP and believes it has respawned, even though the player was never re-registered with the entity manager — producing exactly the kind of ghost/desynced player state this PR fixes elsewhere for AI respawns. Consider checking the boolean return and handling the failure path.</violation>
</file>
<file name="server/ts/player/venice.handler.ts">
<violation number="1" location="server/ts/player/venice.handler.ts:103">
P3: The drop-at-feet block in grantQuestRewards duplicates the create/addItem/broadcast/log pattern already used in handleInventoryDrop (inventory.handler.ts) and equipment.handler.ts; the comment even calls out that it 'mirrors' that pattern. Consider extracting a shared `dropItemAtFeet(world, ctx, kind, properties)` helper to avoid the three near-identical copies drifting apart over time.</violation>
</file>
<file name="server/ts/achievements/achievement.service.ts">
<violation number="1" location="server/ts/achievements/achievement.service.ts:47">
P2: The new idempotent early-return in initPlayer assumes cleanupPlayer() reliably clears a player's map entry before the id is reused; Player.cleanupAchievements() doesn't appear to be wired into any disconnect handler, so a reused player id could now inherit a previous session's achievement state instead of starting fresh (previously the old create-new-empty-state path acted as an implicit safety net). Worth confirming disconnect wiring calls player.cleanupAchievements(), or clearing playerAchievements on disconnect explicitly.</violation>
</file>
<file name="server/ts/world.ts">
<violation number="1" location="server/ts/world.ts:592">
P2: The rift mob spawn tick wraps the entire `forEachPlayer` loop in a single try/catch, so one player's spawn failure aborts spawning for every other player in that tick — unlike the periodic-save loop just above it, which catches per-player. Move the try/catch inside the per-player callback so one player's error doesn't stall rift mob spawns for everyone else in the world.</violation>
</file>
<file name="client/index.html">
<violation number="1" location="client/index.html:206">
P3: The Fracture Rift is accessible via the R key (visible in the button tooltip and wired in main.ts), but the R key shortcut is missing from the keyboard shortcuts help list in index.html. The L key for the boss leaderboard was added alongside it, so it looks like an omission. Adding an entry for R would make the help text consistent with the actual key bindings.</violation>
</file>
<file name="client/ts/network/message-handlers.ts">
<violation number="1" location="client/ts/network/message-handlers.ts:592">
P2: When a quest is completed, the player sees a double notification because both the old legacy handler in message-handlers.ts and the new handler in quest.handler.ts react to the same QUEST_COMPLETE event. The old handler calls `showNotification` with 'Quest Complete! Reward: X' and the new handler calls `showNotification` with 'Reward: X, +Y XP', so two toast-like messages appear. Remove the old QUEST_COMPLETE handler from message-handlers.ts (it was already done correctly for QUEST_OFFER in the same function) — the quest.handler.ts + QuestController.handleQuestComplete already handles the user-facing feedback.</violation>
</file>
<file name="client/ts/ui/minimap-ui.ts">
<violation number="1" location="client/ts/ui/minimap-ui.ts:280">
P3: Minimap item check does not exclude chests from the gold dot rendering. The new code uses `Types.isObject(entity.kind)` which includes chests (CHEST, CHEST_CRATE, etc.), but the game type system considers chests as non-items — `Types.isItem()` explicitly filters them out with `!isChest(kind)`. This means chests now render as gold "item" dots on the minimap when they didn't before. Consider using `Types.isItem(entity.kind)` for consistency with the rest of the game's type classification.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| // Store current NPC for response handling | ||
| this.deps.setCurrentNpcTalk(npc); | ||
| // Register pending talk (keyed by NPC kind) for response handling | ||
| this.deps.setPendingNpcTalk(npc); |
There was a problem hiding this comment.
P2: Keying pending NPC talk purely by npc.kind (not by NPC instance) means talking to two NPCs of the same kind in quick succession (e.g. the map's 10 'guard' NPCs) can cross-attribute the AI response to the wrong NPC, and the first NPC's '...' bubble never resolves since its fallback-timeout equality check fails. Consider keying by a unique NPC id (or kind+id) instead of kind alone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/ts/player/interaction-controller.ts, line 220:
<comment>Keying pending NPC talk purely by `npc.kind` (not by NPC instance) means talking to two NPCs of the same kind in quick succession (e.g. the map's 10 'guard' NPCs) can cross-attribute the AI response to the wrong NPC, and the first NPC's '...' bubble never resolves since its fallback-timeout equality check fails. Consider keying by a unique NPC id (or kind+id) instead of kind alone.</comment>
<file context>
@@ -213,8 +216,8 @@ export class InteractionController {
- // Store current NPC for response handling
- this.deps.setCurrentNpcTalk(npc);
+ // Register pending talk (keyed by NPC kind) for response handling
+ this.deps.setPendingNpcTalk(npc);
// Show thinking indicator
</file context>
| // entity list — the client rebuilt its entity grid on respawn. | ||
| // (No enterCallback: callbacks persist from first login, and a respawn | ||
| // is not a fresh 'join' world event.) | ||
| world.addPlayer(player); |
There was a problem hiding this comment.
P2: If world.addPlayer(player) fails on respawn, the client still receives WELCOME/EQUIP and believes it has respawned, even though the player was never re-registered with the entity manager — producing exactly the kind of ghost/desynced player state this PR fixes elsewhere for AI respawns. Consider checking the boolean return and handling the failure path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/ts/player/auth.handler.ts, line 42:
<comment>If `world.addPlayer(player)` fails on respawn, the client still receives WELCOME/EQUIP and believes it has respawned, even though the player was never re-registered with the entity manager — producing exactly the kind of ghost/desynced player state this PR fixes elsewhere for AI respawns. Consider checking the boolean return and handling the failure path.</comment>
<file context>
@@ -19,13 +19,61 @@ const log = createModuleLogger('AuthHandler');
+ // entity list — the client rebuilt its entity grid on respawn.
+ // (No enterCallback: callbacks persist from first login, and a respawn
+ // is not a fresh 'join' world event.)
+ world.addPlayer(player);
+ world.pushRelevantEntityListTo(player);
+
</file context>
| * the existing entry instead of wiping it with a blank state. | ||
| */ | ||
| initPlayer(playerId: string, savedData?: PlayerAchievements): PlayerAchievements { | ||
| if (!savedData) { |
There was a problem hiding this comment.
P2: The new idempotent early-return in initPlayer assumes cleanupPlayer() reliably clears a player's map entry before the id is reused; Player.cleanupAchievements() doesn't appear to be wired into any disconnect handler, so a reused player id could now inherit a previous session's achievement state instead of starting fresh (previously the old create-new-empty-state path acted as an implicit safety net). Worth confirming disconnect wiring calls player.cleanupAchievements(), or clearing playerAchievements on disconnect explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/ts/achievements/achievement.service.ts, line 47:
<comment>The new idempotent early-return in initPlayer assumes cleanupPlayer() reliably clears a player's map entry before the id is reused; Player.cleanupAchievements() doesn't appear to be wired into any disconnect handler, so a reused player id could now inherit a previous session's achievement state instead of starting fresh (previously the old create-new-empty-state path acted as an implicit safety net). Worth confirming disconnect wiring calls player.cleanupAchievements(), or clearing playerAchievements on disconnect explicitly.</comment>
<file context>
@@ -38,8 +38,19 @@ export class AchievementService {
+ * the existing entry instead of wiping it with a blank state.
*/
initPlayer(playerId: string, savedData?: PlayerAchievements): PlayerAchievements {
+ if (!savedData) {
+ const existing = this.playerAchievements.get(playerId);
+ if (existing) {
</file context>
| // The manager owns cadence (SPAWN_INTERVAL), mob selection and difficulty | ||
| // scaling; the world spawns through the normal addMob path so rift mobs | ||
| // aggro, move, drop loot and broadcast like any other mob. | ||
| this.riftSpawnInterval = setInterval(() => { |
There was a problem hiding this comment.
P2: The rift mob spawn tick wraps the entire forEachPlayer loop in a single try/catch, so one player's spawn failure aborts spawning for every other player in that tick — unlike the periodic-save loop just above it, which catches per-player. Move the try/catch inside the per-player callback so one player's error doesn't stall rift mob spawns for everyone else in the world.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/ts/world.ts, line 592:
<comment>The rift mob spawn tick wraps the entire `forEachPlayer` loop in a single try/catch, so one player's spawn failure aborts spawning for every other player in that tick — unlike the periodic-save loop just above it, which catches per-player. Move the try/catch inside the per-player callback so one player's error doesn't stall rift mob spawns for everyone else in the world.</comment>
<file context>
@@ -557,6 +584,39 @@ export class World {
+ // The manager owns cadence (SPAWN_INTERVAL), mob selection and difficulty
+ // scaling; the world spawns through the normal addMob path so rift mobs
+ // aggro, move, drop loot and broadcast like any other mob.
+ this.riftSpawnInterval = setInterval(() => {
+ try {
+ this.forEachPlayer((player: Player) => {
</file context>
| // second NPC before the first answers no longer eats the first's line | ||
| var npc = game.pendingNpcTalks[npcKind]; | ||
| if (npc) { | ||
| delete game.pendingNpcTalks[npcKind]; |
There was a problem hiding this comment.
P2: When a quest is completed, the player sees a double notification because both the old legacy handler in message-handlers.ts and the new handler in quest.handler.ts react to the same QUEST_COMPLETE event. The old handler calls showNotification with 'Quest Complete! Reward: X' and the new handler calls showNotification with 'Reward: X, +Y XP', so two toast-like messages appear. Remove the old QUEST_COMPLETE handler from message-handlers.ts (it was already done correctly for QUEST_OFFER in the same function) — the quest.handler.ts + QuestController.handleQuestComplete already handles the user-facing feedback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/ts/network/message-handlers.ts, line 592:
<comment>When a quest is completed, the player sees a double notification because both the old legacy handler in message-handlers.ts and the new handler in quest.handler.ts react to the same QUEST_COMPLETE event. The old handler calls `showNotification` with 'Quest Complete! Reward: X' and the new handler calls `showNotification` with 'Reward: X, +Y XP', so two toast-like messages appear. Remove the old QUEST_COMPLETE handler from message-handlers.ts (it was already done correctly for QUEST_OFFER in the same function) — the quest.handler.ts + QuestController.handleQuestComplete already handles the user-facing feedback.</comment>
<file context>
@@ -580,9 +585,11 @@ function setupCombatHandlers(game: Game, client: GameClient): void {
+ // second NPC before the first answers no longer eats the first's line
+ var npc = game.pendingNpcTalks[npcKind];
+ if (npc) {
+ delete game.pendingNpcTalks[npcKind];
if (response) {
game.showBubbleFor(npc, response);
</file context>
| // BEFORE processPurchase so limited stock isn't decremented for a | ||
| // purchase we then abort (stock leak). | ||
| const goesToInventory = Types.isWeapon(itemKind) || Types.isArmor(itemKind) || Types.isExpendableItem(itemKind); | ||
| if (goesToInventory && !ctx.getInventory().hasRoom(itemKind)) { |
There was a problem hiding this comment.
P3: Now that inventory room is verified upfront before processPurchase, the duplicate hasRoom checks inside the isWeapon/isArmor and isConsumable branches further down are unreachable in the normal synchronous flow and could be removed for clarity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/ts/player/shop.handler.ts, line 52:
<comment>Now that inventory room is verified upfront before processPurchase, the duplicate hasRoom checks inside the isWeapon/isArmor and isConsumable branches further down are unreachable in the normal synchronous flow and could be removed for clarity.</comment>
<file context>
@@ -45,6 +45,16 @@ export interface ShopPlayerContext {
+ // BEFORE processPurchase so limited stock isn't decremented for a
+ // purchase we then abort (stock leak).
+ const goesToInventory = Types.isWeapon(itemKind) || Types.isArmor(itemKind) || Types.isExpendableItem(itemKind);
+ if (goesToInventory && !ctx.getInventory().hasRoom(itemKind)) {
+ log.info({ player: ctx.name, itemKind }, 'Inventory is full');
+ ctx.send(new Messages.ShopBuyResult(false, itemKind, ctx.gold, 'Inventory is full').serialize());
</file context>
| * kills (panel review finding). Owner kills already free the slot via | ||
| * recordKill, so this is a no-op for them. | ||
| */ | ||
| freeSpawnSlotForMob(mobId: number): boolean { |
There was a problem hiding this comment.
P3: freeSpawnSlotForMob does a linear scan across all active rift runs for every kill in the game (it's invoked unconditionally from combat-system.ts, not gated on isRiftMob). Consider maintaining a mobId -> playerId index in the manager so this lookup is O(1) instead of O(active runs) per kill.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/ts/rifts/rift-manager.ts, line 341:
<comment>`freeSpawnSlotForMob` does a linear scan across all active rift runs for every kill in the game (it's invoked unconditionally from combat-system.ts, not gated on `isRiftMob`). Consider maintaining a `mobId -> playerId` index in the manager so this lookup is O(1) instead of O(active runs) per kill.</comment>
<file context>
@@ -278,6 +322,65 @@ export class RiftManager {
+ * kills (panel review finding). Owner kills already free the slot via
+ * recordKill, so this is a no-op for them.
+ */
+ freeSpawnSlotForMob(mobId: number): boolean {
+ for (const run of this.activeRuns.values()) {
+ if (run.spawnedMobs.delete(mobId)) return true;
</file context>
| } | ||
|
|
||
| // No inventory surface or inventory full — drop at the player's feet | ||
| const world = ctx.getWorld?.(); |
There was a problem hiding this comment.
P3: The drop-at-feet block in grantQuestRewards duplicates the create/addItem/broadcast/log pattern already used in handleInventoryDrop (inventory.handler.ts) and equipment.handler.ts; the comment even calls out that it 'mirrors' that pattern. Consider extracting a shared dropItemAtFeet(world, ctx, kind, properties) helper to avoid the three near-identical copies drifting apart over time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/ts/player/venice.handler.ts, line 103:
<comment>The drop-at-feet block in grantQuestRewards duplicates the create/addItem/broadcast/log pattern already used in handleInventoryDrop (inventory.handler.ts) and equipment.handler.ts; the comment even calls out that it 'mirrors' that pattern. Consider extracting a shared `dropItemAtFeet(world, ctx, kind, properties)` helper to avoid the three near-identical copies drifting apart over time.</comment>
<file context>
@@ -11,17 +11,104 @@ import { getVeniceService, getFishAudioService } from '../ai';
+ }
+
+ // No inventory surface or inventory full — drop at the player's feet
+ const world = ctx.getWorld?.();
+ if (world && ctx.x !== undefined && ctx.y !== undefined) {
+ const item = world.createItemWithProperties(kind, ctx.x, ctx.y, null);
</file context>
| <li><span class="icon"></span>I - Inventory</li> | ||
| <li><span class="icon"></span>Q - Use health item</li> | ||
| <li><span class="icon"></span>J - Achievements</li> | ||
| <li><span class="icon"></span>L - Boss leaderboard</li> |
There was a problem hiding this comment.
P3: The Fracture Rift is accessible via the R key (visible in the button tooltip and wired in main.ts), but the R key shortcut is missing from the keyboard shortcuts help list in index.html. The L key for the boss leaderboard was added alongside it, so it looks like an omission. Adding an entry for R would make the help text consistent with the actual key bindings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/index.html, line 206:
<comment>The Fracture Rift is accessible via the R key (visible in the button tooltip and wired in main.ts), but the R key shortcut is missing from the keyboard shortcuts help list in index.html. The L key for the boss leaderboard was added alongside it, so it looks like an omission. Adding an entry for R would make the help text consistent with the actual key bindings.</comment>
<file context>
@@ -203,6 +203,7 @@ <h1>
<li><span class="icon"></span>I - Inventory</li>
<li><span class="icon"></span>Q - Use health item</li>
<li><span class="icon"></span>J - Achievements</li>
+ <li><span class="icon"></span>L - Boss leaderboard</li>
<li><span class="icon"></span>M - Minimap</li>
<li><span class="icon"></span>N - Newspaper</li>
</file context>
| // Other players - dark green ink | ||
| ctx.fillStyle = '#3a6b3a'; | ||
| } else if (entity.type === 'item') { | ||
| } else if (Types.isWeapon(entity.kind) || Types.isArmor(entity.kind) || Types.isObject(entity.kind)) { |
There was a problem hiding this comment.
P3: Minimap item check does not exclude chests from the gold dot rendering. The new code uses Types.isObject(entity.kind) which includes chests (CHEST, CHEST_CRATE, etc.), but the game type system considers chests as non-items — Types.isItem() explicitly filters them out with !isChest(kind). This means chests now render as gold "item" dots on the minimap when they didn't before. Consider using Types.isItem(entity.kind) for consistency with the rest of the game's type classification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/ts/ui/minimap-ui.ts, line 280:
<comment>Minimap item check does not exclude chests from the gold dot rendering. The new code uses `Types.isObject(entity.kind)` which includes chests (CHEST, CHEST_CRATE, etc.), but the game type system considers chests as non-items — `Types.isItem()` explicitly filters them out with `!isChest(kind)`. This means chests now render as gold "item" dots on the minimap when they didn't before. Consider using `Types.isItem(entity.kind)` for consistency with the rest of the game's type classification.</comment>
<file context>
@@ -264,15 +268,16 @@ export class MinimapUI {
// Other players - dark green ink
ctx.fillStyle = '#3a6b3a';
- } else if (entity.type === 'item') {
+ } else if (Types.isWeapon(entity.kind) || Types.isArmor(entity.kind) || Types.isObject(entity.kind)) {
ctx.fillStyle = '#8b7a2a'; // Gold ink for items
} else {
</file context>
Suggested change
| } else if (Types.isWeapon(entity.kind) || Types.isArmor(entity.kind) || Types.isObject(entity.kind)) { | |
| } else if (Types.isItem(entity.kind)) { |
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/ts/main.ts">
<violation number="1" location="client/ts/main.ts:450">
P2: `hadOpenUi` doesn't account for shop/inventory/newspaper/quest-tracker panels that `app.hideWindows()` also closes, so ESC will still incorrectly call `game.exitRift()` when one of those panels (not achievements/instructions/credits/rift) was open during an active rift — reintroducing the same 'ESC exits rift with window open' bug this change is meant to fix for those other UI panels.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Comment on lines
+450
to
+453
| const hadOpenUi = !!(game.achievementUI?.isVisible?.()) || | ||
| $('#instructions').hasClass('active') || | ||
| $('body').hasClass('credits') || | ||
| !!(game.riftUI?.hasOpenPanel?.()); |
There was a problem hiding this comment.
P2: hadOpenUi doesn't account for shop/inventory/newspaper/quest-tracker panels that app.hideWindows() also closes, so ESC will still incorrectly call game.exitRift() when one of those panels (not achievements/instructions/credits/rift) was open during an active rift — reintroducing the same 'ESC exits rift with window open' bug this change is meant to fix for those other UI panels.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/ts/main.ts, line 450:
<comment>`hadOpenUi` doesn't account for shop/inventory/newspaper/quest-tracker panels that `app.hideWindows()` also closes, so ESC will still incorrectly call `game.exitRift()` when one of those panels (not achievements/instructions/credits/rift) was open during an active rift — reintroducing the same 'ESC exits rift with window open' bug this change is meant to fix for those other UI panels.</comment>
<file context>
@@ -445,10 +445,16 @@ var initGame = function () {
if (key === 27) { // ESC
+ // Only exit an active rift when ESC closed nothing: if any window or
+ // rift panel was open, ESC's job was to close it, not end the run
+ const hadOpenUi = !!(game.achievementUI?.isVisible?.()) ||
+ $('#instructions').hasClass('active') ||
+ $('body').hasClass('credits') ||
</file context>
Suggested change
| const hadOpenUi = !!(game.achievementUI?.isVisible?.()) || | |
| $('#instructions').hasClass('active') || | |
| $('body').hasClass('credits') || | |
| !!(game.riftUI?.hasOpenPanel?.()); | |
| const hadOpenUi = !!(game.achievementUI?.isVisible?.()) || | |
| $('#instructions').hasClass('active') || | |
| $('body').hasClass('credits') || | |
| $('body').hasClass('about') || | |
| !!(game.shopUI?.isOpen?.()) || | |
| !!(game.inventoryUI?.isVisible?.()) || | |
| !!(game.uiManager?.isNewspaperVisible?.()) || | |
| !!(game.questUI?.isTrackerVisible?.()) || | |
| !!(game.riftUI?.hasOpenPanel?.()); |
Leaderboard playerName is user-controlled and went raw into innerHTML — a crafted character name would XSS every leaderboard viewer. Modifier name/description get the same treatment (title-attribute context). Colors stay raw: they come from the static MODIFIERS table, not users.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/ts/player/interaction-controller.ts (1)
267-270: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the remaining production click-debug logging.
The visible overlay was removed, but the
[Click Debug]console.logstill runs on every click. Delete it or gate it behind a debug flag so the client no longer ships production debug output.🤖 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 `@client/ts/player/interaction-controller.ts` around lines 267 - 270, Remove the “[Click Debug]” console.log from the click handling flow in interaction-controller.ts, or gate it behind the existing debug flag so production builds emit no click-debug output. Preserve the surrounding entity and position handling.
🧹 Nitpick comments (3)
server/ts/__tests__/message-router.test.ts (1)
604-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQUEST_ABANDON tests share static quest-service state across test order.
getStaticServices().questsis a module-level singleton. The "no active quest" test (Line 618) implicitly relies on the preceding test (Line 604) having already abandoned the quest for the samectx.id; there's no explicit cleanup between tests. If test execution order changes, this becomes an ordering dependency rather than an independent assertion. Consider abandoning/clearing the quest forctx.idin anafterEach/beforeEachto make each test self-contained.🤖 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 `@server/ts/__tests__/message-router.test.ts` around lines 604 - 623, Make the QUEST_ABANDON tests independent of execution order by clearing the static quest state for ctx.id in shared setup or teardown, using getStaticServices().quests and its existing quest-clearing/abandon operation. Ensure each test starts with no active quest while preserving both assertions.server/ts/player/auth.handler.ts (1)
22-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the respawn branch.
The graph evidence for
auth.handler.test.tsonly shows calls exercising the normal (non-respawn)HELLOpath (new/existing character, wrong password, etc.). This new respawn branch (hasEnteredGame && isDead) is a large behavioral fork — it skips storage reload, achievement/inventory/skill re-init, andenterCallback, while re-adding the player and resendingWELCOME/EQUIP. A dedicated test (sethasEnteredGame=true,isDead=true, callhandleHelloagain, assertworld.addPlayerwas called,isDeadis now false,WELCOME/EQUIPwere resent, and storage/achievement init were not called) would guard this path against regressions.🤖 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 `@server/ts/player/auth.handler.ts` around lines 22 - 76, Add a dedicated auth.handler test for the hasEnteredGame && isDead respawn branch, configuring an authenticated dead player before calling handleHello again. Assert the player is revived, world.addPlayer is called, WELCOME and available EQUIP messages are resent, and storage reload plus achievement/inventory/skill initialization and enterCallback are not invoked.server/ts/player/rift.handler.ts (1)
52-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRift-entry failure reason is not discriminated.
riftManager.startRunreturnsnullfor two different causes (already in a rift vs. level too low), sohandleRiftEnteralways sends the same combined string regardless of which one applied. Since the PR's stated goal is showing the server-provided reason to the player, consider havingstartRunreturn a discriminated failure so the client message is precise.♻️ Suggested direction
- startRun(playerId: number, playerName: string, playerLevel: number): ActiveRun | null { + startRun(playerId: number, playerName: string, playerLevel: number): { run: ActiveRun } | { error: 'already_in_rift' | 'level_too_low' } { if (this.activeRuns.has(playerId)) { - return null; + return { error: 'already_in_rift' }; } ... if (playerLevel < tier.minLevel) { - return null; + return { error: 'level_too_low' }; } - return run; + return { run };Then map the discriminant to a specific message in
handleRiftEnter.🤖 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 `@server/ts/player/rift.handler.ts` around lines 52 - 63, Update riftManager.startRun and handleRiftEnter to return and consume a discriminated failure result instead of using null for both cases. Map the discriminant to a precise player-facing message for already-in-rift versus insufficient-level failures, while preserving the existing successful run path and failure response structure.
🤖 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 `@client/ts/audio.ts`:
- Line 20: Add the missing client/audio/sounds/glitch1.ogg asset corresponding
to the existing glitch1 entry in soundNames, ensuring the fallback preload path
can load it and complete the audio startup flow.
In `@client/ts/main.ts`:
- Around line 448-459: Update the hadOpenUi guard in the ESC handler to also
detect the shop, inventory, newspaper, and quest interfaces that
app.hideWindows() closes. Preserve the existing checks and ensure any of these
windows being open prevents game.exitRift() after ESC.
In `@client/ts/player/interaction-controller.ts`:
- Around line 49-53: Update the pending NPC-talk accessor types in the
interaction-controller interface: change setPendingNpcTalk to accept Character
or Npc, and change getPendingNpcTalk to return Character | undefined. Keep
clearPendingNpcTalk’s npcKind parameter unchanged and ensure the implementations
and callers conform to the typed pendingNpcTalks record.
In `@server/ts/rifts/rift-manager.ts`:
- Around line 66-71: Update the `RIFT_BOUNDS` coordinates so the rift arena lies
outside the lavaland zone and rift mobs cannot receive normal zone bonuses
through their spawn positions; preserve the arena’s existing bounds structure
and dimensions.
---
Outside diff comments:
In `@client/ts/player/interaction-controller.ts`:
- Around line 267-270: Remove the “[Click Debug]” console.log from the click
handling flow in interaction-controller.ts, or gate it behind the existing debug
flag so production builds emit no click-debug output. Preserve the surrounding
entity and position handling.
---
Nitpick comments:
In `@server/ts/__tests__/message-router.test.ts`:
- Around line 604-623: Make the QUEST_ABANDON tests independent of execution
order by clearing the static quest state for ctx.id in shared setup or teardown,
using getStaticServices().quests and its existing quest-clearing/abandon
operation. Ensure each test starts with no active quest while preserving both
assertions.
In `@server/ts/player/auth.handler.ts`:
- Around line 22-76: Add a dedicated auth.handler test for the hasEnteredGame &&
isDead respawn branch, configuring an authenticated dead player before calling
handleHello again. Assert the player is revived, world.addPlayer is called,
WELCOME and available EQUIP messages are resent, and storage reload plus
achievement/inventory/skill initialization and enterCallback are not invoked.
In `@server/ts/player/rift.handler.ts`:
- Around line 52-63: Update riftManager.startRun and handleRiftEnter to return
and consume a discriminated failure result instead of using null for both cases.
Map the discriminant to a precise player-facing message for already-in-rift
versus insufficient-level failures, while preserving the existing successful run
path and failure response structure.
🪄 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: 5f67fdd7-3e2e-4a9d-a652-283d1b5fd2cb
📒 Files selected for processing (62)
client/css/main.cssclient/index.htmlclient/ts/__tests__/quest-controller.test.tsclient/ts/audio.tsclient/ts/bootstrap/game-bootstrap.tsclient/ts/game.tsclient/ts/handlers/game-event.handler.tsclient/ts/handlers/rift-ui.handler.tsclient/ts/main.tsclient/ts/network/client-events.tsclient/ts/network/gameclient.tsclient/ts/network/message-handlers.tsclient/ts/player/interaction-controller.tsclient/ts/quest/quest-controller.tsclient/ts/ui/minimap-ui.tsclient/ts/ui/rift-ui.tsserver/ts/__tests__/achievement.handler.test.tsserver/ts/__tests__/aiplayer.test.tsserver/ts/__tests__/auth.handler.test.tsserver/ts/__tests__/character.test.tsserver/ts/__tests__/combat-system.test.tsserver/ts/__tests__/combat.handler.test.tsserver/ts/__tests__/economy.service.test.tsserver/ts/__tests__/format.test.tsserver/ts/__tests__/inventory.handler.test.tsserver/ts/__tests__/map.test.tsserver/ts/__tests__/message-router.test.tsserver/ts/__tests__/message.test.tsserver/ts/__tests__/player.test.tsserver/ts/__tests__/progression.service.test.tsserver/ts/__tests__/rift-manager.test.tsserver/ts/__tests__/rift.handler.test.tsserver/ts/__tests__/roaming-boss.test.tsserver/ts/__tests__/shop.handler.test.tsserver/ts/__tests__/storage.service.test.tsserver/ts/__tests__/venice.handler.test.tsserver/ts/__tests__/world.test.tsserver/ts/achievements/achievement.service.tsserver/ts/ai/aiplayer.tsserver/ts/ai/quest.service.tsserver/ts/ai/types.tsserver/ts/character.tsserver/ts/combat/combat-system.tsserver/ts/format.tsserver/ts/main.tsserver/ts/map.tsserver/ts/message.tsserver/ts/player.tsserver/ts/player/auth.handler.tsserver/ts/player/combat.handler.tsserver/ts/player/economy.service.tsserver/ts/player/inventory.handler.tsserver/ts/player/message-router.tsserver/ts/player/progression.service.tsserver/ts/player/rift.handler.tsserver/ts/player/shop.handler.tsserver/ts/player/venice.handler.tsserver/ts/rifts/rift-manager.tsserver/ts/roaming-boss.tsserver/ts/world.tsshared/ts/domain/game-constants.tsshared/ts/gametypes.ts
| areas = []; | ||
| musicNames = ['village', 'beach', 'forest', 'cave', 'desert', 'lavaland', 'boss', 'graveyard']; | ||
| soundNames = ['loot', 'hit1', 'hit2', 'hurt', 'heal', 'chat', 'revive', 'death', 'firefox', 'achievement', 'kill1', 'kill2', 'noloot', 'teleport', 'chest', 'npc', 'npc-end', 'levelup', 'gold', 'equip', 'quest']; | ||
| soundNames = ['loot', 'hit1', 'hit2', 'hurt', 'heal', 'chat', 'revive', 'death', 'firefox', 'achievement', 'kill1', 'kill2', 'noloot', 'teleport', 'chest', 'npc', 'npc-end', 'levelup', 'gold', 'equip', 'quest', 'glitch1']; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for ext in mp3 ogg; do
if ! git ls-files | grep -Eiq "(^|/)glitch1\.${ext}$"; then
echo "Missing tracked glitch1.${ext} asset"
exit 1
fi
doneRepository: georgeglarson/Fracture
Length of output: 194
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '\n-- audio.ts --\n'
cat -n client/ts/audio.ts | sed -n '1,220p'
printf '\n-- glitch1 assets tracked --\n'
git ls-files | rg -n '(^|/)glitch1\.(mp3|ogg)$' || true
printf '\n-- any references to glitch1 --\n'
rg -n 'glitch1' client -SRepository: georgeglarson/Fracture
Length of output: 8516
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Show the loading helpers in client/ts/audio.ts
cat -n client/ts/audio.ts | sed -n '220,360p'
# Find the load callbacks for sounds/music
rg -n "canplaythrough|error|loaded_callback|loadSound|loadMusic|load\(" client/ts/audio.ts -n -SRepository: georgeglarson/Fracture
Length of output: 6154
Add client/audio/sounds/glitch1.ogg
client/ts/audio.ts falls back to ogg when MP3 isn’t supported, but only client/audio/sounds/glitch1.mp3 is tracked. On those browsers the preload counter never clears, so music startup stalls.
🤖 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 `@client/ts/audio.ts` at line 20, Add the missing
client/audio/sounds/glitch1.ogg asset corresponding to the existing glitch1
entry in soundNames, ensuring the fallback preload path can load it and complete
the audio startup flow.
Comment on lines
+448
to
+459
| // Only exit an active rift when ESC closed nothing: if any window or | ||
| // rift panel was open, ESC's job was to close it, not end the run | ||
| const hadOpenUi = !!(game.achievementUI?.isVisible?.()) || | ||
| $('#instructions').hasClass('active') || | ||
| $('body').hasClass('credits') || | ||
| !!(game.riftUI?.hasOpenPanel?.()); | ||
| app.hideWindows(); | ||
| if (game.riftUI?.hasOpenPanel?.()) { | ||
| game.riftUI.closePanels(); | ||
| } else if (!hadOpenUi && game.riftUI?.isActive()) { | ||
| game.exitRift(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "hideWindows" --type=ts client/tsRepository: georgeglarson/Fracture
Length of output: 435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the hideWindows implementation and nearby UI APIs.
sed -n '360,460p' client/ts/app.ts
printf '\n---\n'
# Find visibility/open-state methods on relevant UI managers.
rg -n "isVisible\\(|hasOpenPanel\\(|hideWindows\\(|showWindows\\(|closePanels\\(" client/tsRepository: georgeglarson/Fracture
Length of output: 4950
Include the other ESC-closable windows in hadOpenUi
hadOpenUi only accounts for achievements, instructions, credits, and rift panels. Since app.hideWindows() also closes shop, inventory, newspaper, and quest UIs, opening any of those while in a rift still falls through to game.exitRift() after ESC closes the window. Add those checks to the guard so ESC doesn’t end the run after closing another panel.
🤖 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 `@client/ts/main.ts` around lines 448 - 459, Update the hadOpenUi guard in the
ESC handler to also detect the shop, inventory, newspaper, and quest interfaces
that app.hideWindows() closes. Preserve the existing checks and ensure any of
these windows being open prevents game.exitRift() after ESC.
Comment on lines
+49
to
+53
| // Pending NPC talks, keyed by NPC kind (the server echoes the kind in | ||
| // NPCTALK_RESPONSE, so responses can be attributed to the right NPC) | ||
| setPendingNpcTalk: (npc: any) => void; | ||
| getPendingNpcTalk: (npcKind: number) => any; | ||
| clearPendingNpcTalk: (npcKind: number) => void; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files client/ts/player/interaction-controller.ts client/ts/**/message-handlers.ts client/ts/**/game*.ts client/ts/**/player*.ts | sed -n '1,200p'Repository: georgeglarson/Fracture
Length of output: 421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
client/ts/player/interaction-controller.ts
client/ts/game.ts
client/ts/player/message-handlers.ts
)
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "### $f"
wc -l "$f"
echo
cat -n "$f" | sed -n '1,240p'
echo
fi
doneRepository: georgeglarson/Fracture
Length of output: 19955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, subprocess, textwrap, sys
paths = [
"client/ts/player/interaction-controller.ts",
"client/ts/game.ts",
"client/ts/player/message-handlers.ts",
]
for p in paths:
if os.path.exists(p):
print(f"### {p}")
with open(p, "r", encoding="utf-8") as f:
lines = f.readlines()
print(f"lines={len(lines)}")
for i, line in enumerate(lines[:260], start=1):
print(f"{i:4d}: {line.rstrip()}")
print()
PYRepository: georgeglarson/Fracture
Length of output: 20671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' client/ts/player/interaction-controller.tsRepository: georgeglarson/Fracture
Length of output: 6477
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '### pendingNpcTalk references'
rg -n "pendingNpcTalk|setPendingNpcTalk|getPendingNpcTalk|clearPendingNpcTalk" client/ts shared/ts -S
printf '\n%s\n' '### interaction-controller surrounding code'
sed -n '200,245p' client/ts/player/interaction-controller.ts
printf '\n%s\n' '### message-handlers around NPC talk handling'
sed -n '1,260p' client/ts/network/message-handlers.ts
printf '\n%s\n' '### game.ts pending-talk related methods'
sed -n '520,760p' client/ts/game.tsRepository: georgeglarson/Fracture
Length of output: 19951
Type the pending NPC-talk accessors
Game.pendingNpcTalks is already Record<number, Character>, so setPendingNpcTalk should accept Character/Npc and getPendingNpcTalk should return Character | undefined instead of any.
🧰 Tools
🪛 ESLint
[error] 51-51: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 52-52: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 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 `@client/ts/player/interaction-controller.ts` around lines 49 - 53, Update the
pending NPC-talk accessor types in the interaction-controller interface: change
setPendingNpcTalk to accept Character or Npc, and change getPendingNpcTalk to
return Character | undefined. Keep clearPendingNpcTalk’s npcKind parameter
unchanged and ensure the implementations and callers conform to the typed
pendingNpcTalks record.
Source: Linters/SAST tools
Comment on lines
66
to
71
| private readonly RIFT_BOUNDS = { // Virtual rift arena bounds | ||
| minX: 10, | ||
| maxX: 30, | ||
| minY: 10, | ||
| maxY: 30 | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the map/zone data for whatever occupies (10-30, 10-30)
rg -n "resolveZoneId|ZONE_DATA" server/ts/zones -n
fd . server/ts/zones --type fRepository: georgeglarson/Fracture
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect zone definitions and the logic that maps coordinates to zones.
ast-grep outline server/ts/zones/zone-manager.ts --view expanded || true
ast-grep outline server/ts/zones/index.ts --view expanded || true
echo '--- zone-manager.ts ---'
cat -n server/ts/zones/zone-manager.ts | sed -n '1,260p'
echo '--- zones/index.ts ---'
cat -n server/ts/zones/index.ts | sed -n '1,260p'Repository: georgeglarson/Fracture
Length of output: 6428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any zone rectangle/coordinate data in the repo.
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
"minX|maxX|minY|maxY|rect|rectangle|bounds|zone.*x|zone.*y|zoneId|resolveZoneId|walkable|arena" server/tsRepository: georgeglarson/Fracture
Length of output: 40705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared zone definitions and the coordinate resolver.
ast-grep outline shared/ts/zones.ts --view expanded || true
ast-grep outline server/ts/combat/aggro-policy.ts --view expanded || true
echo '--- shared/ts/zones.ts ---'
cat -n shared/ts/zones.ts | sed -n '1,320p'
echo '--- server/ts/combat/aggro-policy.ts ---'
cat -n server/ts/combat/aggro-policy.ts | sed -n '1,280p'Repository: georgeglarson/Fracture
Length of output: 1330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the shared zone definition file and inspect the resolver implementation.
fd -a "zones" . | sed -n '1,50p'
rg -n "export const ZONE_DATA|interface ZoneDefinition|getZoneAtPosition|resolveZoneId" . --glob '!node_modules' --glob '!dist' --glob '!build'
# Print the relevant section of the resolver file.
cat -n server/ts/combat/aggro-policy.ts | sed -n '200,280p'Repository: georgeglarson/Fracture
Length of output: 4373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the zone tests around the coordinates and boundaries that matter here.
cat -n server/ts/__tests__/zone.handler.test.ts | sed -n '220,340p'
cat -n server/ts/__tests__/aggro-policy.test.ts | sed -n '450,490p'Repository: georgeglarson/Fracture
Length of output: 6600
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the rift arena is explicitly carved out as a separate walkable pocket.
rg -n "rift arena|RIFT_BOUNDS|walkable tiles|teleport-in point|arena center|pocket|isolated" server/ts server/ts/__tests__ --glob '!node_modules' --glob '!dist' --glob '!build'Repository: georgeglarson/Fracture
Length of output: 5144
Rift arena overlaps lavaland — RIFT_BOUNDS falls inside the lavaland zone, so rift mobs can inherit normal zone XP/gold/drop bonuses through their real spawn coordinates. If the arena is meant to be isolated, move it outside the live map zone or bypass zone lookup for rift runs.
🤖 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 `@server/ts/rifts/rift-manager.ts` around lines 66 - 71, Update the
`RIFT_BOUNDS` coordinates so the rift arena lies outside the lavaland zone and
rift mobs cannot receive normal zone bonuses through their spawn positions;
preserve the arena’s existing bounds structure and dimensions.
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
Follow-up to the 2026-07-20 gameplay-correctness scan (6-lane end-to-end trace). The scan found the codebase's signature defect at scale: fully-built features never wired, and features advertising behavior they don't have. This PR fixes the high/medium findings in three server/client lanes plus the namesake feature itself.
Quest UX (was the 60-second CEO experience, broken):
Messages.QuestStatusafter offers and each increment, both AI and no-AI branches)QUEST_ABANDON(105) routed to the quest service +QuestStatus(null)ackState-wipe bugs:
Economy/inventory: cake + firepotion purchases no longer phantom, shop stock no longer leaks on abort, unequip preserves properties + notifies owner, stackable drops no longer desync
Combat/progression (advertised-but-fake class):
bossIdnow set)Client truth pass:
#tap-debugoverlay removed from production, Venice branding retired, boss spawn announcements display, boss leaderboard reachable (L key), HUD equipment icons refresh, minimap fixes (incl. returning-player auto-show), glitch1 sound, quest offer de-duplicated, NPC dialogue keyed by kind, I/J keys guarded, ESC closes rift panels before exiting a runThe Fracture Rift (was completely unreachable): format registration, entry via status-bar button + R key + ESC, positional
Messages.*classes, world spawn tick with map-validated positions, kill/death/disconnect wiring (disconnect restores entry position BEFORE save — panel finding), off-owner kills free the owner's spawn slot (panel finding), door-path teleport, SQLite-backed leaderboard, FORTIFIED/EMPOWERED modifiers at spawnTest plan
pnpm test: 70 files / 3,321 tests green (+122 vs master)pnpm build:server(tsc) +pnpm build:client(webpack) both clean; eslint parity on touched filesSummary by Sourcery
Restore advertised gameplay features and wiring across quests, combat, progression, AI, rifts, and client UI, including a fully playable Fracture Rift with persistent leaderboard and correct server-client messaging.
New Features:
Bug Fixes:
Enhancements:
Deployment:
Tests:
Summary by cubic
Fixes core gameplay wiring and ships a fully working Fracture Rift. Quests, combat, bosses, economy, and client UI now behave as advertised, and player progress no longer gets wiped on respawn/login.
New Features
Bug Fixes
#tap-debug; retired Venice branding; minimap visibility/dots fixed; HUD equipment icons refresh on live equip; NPC talks keyed by kind; I/J keys gated until game start; ESC exits a rift only when no window or panel was closed (rift panels close first); failed rift entries show the server’s reason; boss spawn announcements and leaderboard reachable; rift UI escapes leaderboard names and modifier text to prevent XSS.Written for commit 85c45fe. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation