Merge pull request #238 from simoleo89/fix/inventory-state-bugs

fix(inventory): state-mutation & stale-read bugs

src/hooks/inventory/useInventoryBadges.ts

@@ -1,5 +1,5 @@
 import { BadgeReceivedEvent, BadgesEvent, RequestBadgesComposer, SetActivatedBadgesComposer } from '@nitrots/nitro-renderer';
-import { useEffect, useRef, useState } from 'react';
+import { useEffect, useState } from 'react';
 import { useBetween } from 'use-between';
 import { GetConfigurationValue, SendMessageComposer, UnseenItemCategory } from '../../api';
 import { useMessageEvent } from '../events';
@@ -17,7 +17,6 @@ const useInventoryBadgesState = () =>
     const { isUnseen = null, resetCategory = null } = useInventoryUnseenTracker();
 
     const maxBadgeCount = GetConfigurationValue<number>('user.badges.max.slots', 5);
-    const pendingUpdatesRef = useRef(0);
     const isWearingBadge = (badgeCode: string) => activeBadgeCodes.some(code => code === badgeCode);
     const canWearBadges = () => (activeBadgeCodes.filter(Boolean).length < maxBadgeCount);
 
@@ -35,7 +34,6 @@ const useInventoryBadgesState = () =>
 
     const sendActiveBadges = (badges: (string | null)[]) =>
     {
-        pendingUpdatesRef.current++;
         const composer = new SetActivatedBadgesComposer();
         for(let i = 0; i < maxBadgeCount; i++) composer.addActivatedBadge(badges[i] ?? '');
         SendMessageComposer(composer);
@@ -93,16 +91,14 @@ const useInventoryBadgesState = () =>
             return newValue;
         });
 
-        // Skip overwriting activeBadgeCodes if we have pending local changes
+        // The emulator answers SetActivatedBadges (UserWearBadgeEvent) with a
-        if(pendingUpdatesRef.current > 0)
+        // UserBadgesComposer room broadcast, NOT a BadgesEvent — so there is no
-        {
+        // echo to suppress and the old pendingUpdatesRef counter only ever
-            pendingUpdatesRef.current--;
+        // leaked (incremented on every edit, never decremented), which then
-        }
+        // silently dropped legitimate later BadgesEvent updates. The server is
-        else
+        // authoritative here (edits are already persisted), so always apply it.
-        {
         const serverBadges = parser.getActiveBadgeCodes();
         setActiveBadgeCodes(toFixedSlots(serverBadges));
-        }
 
         setBadgeCodes(allBadgeCodes);
     });

src/hooks/inventory/useInventoryPrefixes.ts

@@ -78,25 +78,29 @@ const useInventoryPrefixesState = () =>
 
         setPrefixes(prevValue =>
         {
-            return prevValue.map(p => ({
+            const next = prevValue.map(p => ({
                 ...p,
                 active: p.id === parser.prefixId
             }));
-        });
 
+            // Derive the active prefix from the freshly-mapped list, not from
+            // the `prefixes` closure (which lags a render and is stale when the
+            // prefix was added earlier in the same event batch).
             if(parser.prefixId === 0)
             {
                 setActivePrefix(null);
             }
             else
             {
-            setActivePrefix(prev =>
+                const found = next.find(p => p.id === parser.prefixId);
-            {
+
-                const found = prefixes.find(p => p.id === parser.prefixId);
+                setActivePrefix(found
-                if(found) return { ...found, active: true, font: parser.font || found.font || '' };
+                    ? { ...found, active: true, font: parser.font || found.font || '' }
-                return { id: parser.prefixId, text: parser.text, color: parser.color, icon: parser.icon || '', effect: parser.effect || '', font: parser.font || '', active: true };
+                    : { id: parser.prefixId, text: parser.text, color: parser.color, icon: parser.icon || '', effect: parser.effect || '', font: parser.font || '', active: true });
-            });
             }
+
+            return next;
+        });
     });
 
     const activatePrefix = (prefixId: number) =>

src/hooks/inventory/useInventoryUnseenTracker.ts

@@ -63,7 +63,10 @@ const useInventoryUnseenTrackerState = () =>
             const newValue = new Map(prevValue);
             const existing = newValue.get(category);
 
-            if(existing) for(const itemId of itemIds) existing.splice(existing.indexOf(itemId), 1);
+            // Replace the per-category array instead of splicing the one still
+            // referenced by the previous Map, and filter (an absent id used to
+            // splice(indexOf=-1) and drop the wrong last element).
+            if(existing) newValue.set(category, existing.filter(id => !itemIds.includes(id)));
 
             sendResetItemsMessage(category, itemIds);
 
@@ -90,9 +93,9 @@ const useInventoryUnseenTrackerState = () =>
 
             const newValue = new Map(prevValue);
             const items = newValue.get(category);
-            const index = items.indexOf(itemId);
 
-            if(index >= 0) items.splice(index, 1);
+            // Clone the array rather than splicing the one shared with prevValue.
+            if(items && items.indexOf(itemId) >= 0) newValue.set(category, items.filter(id => id !== itemId));
 
             return newValue;
         });
@@ -108,18 +111,15 @@ const useInventoryUnseenTrackerState = () =>
 
             for(const category of parser.categories)
             {
-                let existing = newValue.get(category);
+                // Clone the existing array so we never push into the one still
-
+                // referenced by the previous (shallow-copied) Map.
-                if(!existing)
+                const merged = [ ...(newValue.get(category) ?? []) ];
-                {
-                    existing = [];
-
-                    newValue.set(category, existing);
-                }
 
                 const itemIds = parser.getItemsByCategory(category);
 
-                for(const itemId of itemIds) ((existing.indexOf(itemId) === -1) && existing.push(itemId));
+                for(const itemId of itemIds) if(merged.indexOf(itemId) === -1) merged.push(itemId);
+
+                newValue.set(category, merged);
             }
 
             return newValue;