[4k] REWORK inventory GUI : Spread, Anvil, Enchant etc#518
[4k] REWORK inventory GUI : Spread, Anvil, Enchant etc#518
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a React-based Inventory overlay using the Changes
Sequence Diagram(s)sequenceDiagram
participant App as App/UI
participant InventoryComp as Inventory Component
participant Connector as Mineflayer Connector
participant Providers as Texture/Scale/Inventory Providers
participant Overlay as Inventory Overlay (JEI)
participant ResourcePack as ResourcePack Manager
App->>InventoryComp: render <Inventory/>
InventoryComp->>ResourcePack: read textures / subscribe assets events
InventoryComp->>Connector: createMineflayerConnector(bot, itemMapper) [memoized]
Connector-->>InventoryComp: connector instance
InventoryComp->>Providers: wrap overlay with texture & scale contexts
InventoryComp->>Overlay: render overlay (type from modal stack)
Overlay->>InventoryComp: request item recipes/usages
InventoryComp->>InventoryComp: call getItemRecipes / getItemUsages
ResourcePack->>InventoryComp: assetsTexturesUpdated / assetsInventoryReady
InventoryComp->>InventoryComp: clearInventoryCaches(), bump textureVersion, recreate connector if needed
Overlay-->>App: UI rendered / user interactions (close)
Overlay->>InventoryComp: handleClose -> connector close + hide modal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/react/inventory/Inventory.tsx (2)
11-11: Remove unusedwindowTypevariable.The
windowTypeconstant is declared but never used in this component.♻️ Proposed fix
export const Inventory = () => { - const windowType = 'player' const connector = useMemo(() => createMineflayerConnector(bot as MineflayerBot), [bot])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/inventory/Inventory.tsx` at line 11, The Inventory component declares an unused constant windowType = 'player'; remove this dead variable from the component (delete the windowType declaration) to clean up unused code and avoid linter warnings; search for windowType in the Inventory function to ensure it's not referenced elsewhere before removing.
1-1: Consider importing from the package root instead of/src.Importing directly from
'minecraft-inventory/src'couples this code to the package's internal structure. If the package has proper exports configured, prefer importing from'minecraft-inventory'. This reduces brittleness if the package's internal structure changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/inventory/Inventory.tsx` at line 1, The import at the top currently pulls internal modules from 'minecraft-inventory/src'; change it to import the same symbols (TextureProvider, ScaleProvider, InventoryProvider, InventoryOverlay, createMineflayerConnector, MineflayerBot, InventoryOverlayProps) from the package root 'minecraft-inventory' so the code depends on the public API rather than internal paths; update the import statement to reference 'minecraft-inventory' and keep the same named imports to avoid breaking usages elsewhere in Inventory.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react/inventory/Inventory.tsx`:
- Line 12: Guard against bot being undefined by only calling
createMineflayerConnector when bot is present and return a safe fallback (e.g.,
null) otherwise; update the useMemo for connector (the const connector =
useMemo(...) line) to check the global bot before invoking
createMineflayerConnector and ensure connector is typed/handled for null. Also
stop relying solely on the global bot in the dependency array: wire the
Inventory component to a React-tracked source (e.g., local state or context that
holds the bot) or subscribe to bot changes in useEffect so re-renders occur when
a new bot is assigned, and use that tracked value in the useMemo dependency
instead of the raw global.
---
Nitpick comments:
In `@src/react/inventory/Inventory.tsx`:
- Line 11: The Inventory component declares an unused constant windowType =
'player'; remove this dead variable from the component (delete the windowType
declaration) to clean up unused code and avoid linter warnings; search for
windowType in the Inventory function to ensure it's not referenced elsewhere
before removing.
- Line 1: The import at the top currently pulls internal modules from
'minecraft-inventory/src'; change it to import the same symbols
(TextureProvider, ScaleProvider, InventoryProvider, InventoryOverlay,
createMineflayerConnector, MineflayerBot, InventoryOverlayProps) from the
package root 'minecraft-inventory' so the code depends on the public API rather
than internal paths; update the import statement to reference
'minecraft-inventory' and keep the same named imports to avoid breaking usages
elsewhere in Inventory.tsx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23a521d2-6614-4c89-b632-ac26097127dc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonsrc/react/inventory/Inventory.tsxsrc/reactUi.tsx
src/react/inventory/Inventory.tsx
Outdated
|
|
||
| export const Inventory = () => { | ||
| const windowType = 'player' | ||
| const connector = useMemo(() => createMineflayerConnector(bot as MineflayerBot), [bot]) |
There was a problem hiding this comment.
Guard against bot being undefined to prevent runtime crash.
The global bot variable can be undefined during disconnect or before connection (see src/index.ts where bot = undefined and src/globals.js where it starts as undefined). Calling createMineflayerConnector(bot as MineflayerBot) when bot is undefined will likely crash.
Additionally, useMemo with [bot] as a dependency may not re-trigger correctly since bot is a global variable that React doesn't track. When a new bot instance is assigned, this component won't automatically re-render.
🛡️ Proposed fix for null safety
export const Inventory = () => {
const windowType = 'player'
+ const { inventory } = useSnapshot(openInventoryProxy) as typeof openInventoryProxy
+
+ // Early return before creating connector if no bot or inventory
+ if (!inventory || !bot) return null
+
const connector = useMemo(() => createMineflayerConnector(bot as MineflayerBot), [bot])
const appScale = useAppScale()
- const { inventory } = useSnapshot(openInventoryProxy) as typeof openInventoryProxy
-
- if (!inventory) return nullNote: The useMemo dependency on global bot may still not work as expected for re-renders. Consider using a state or context pattern to track the bot instance if reconnection scenarios need to be supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react/inventory/Inventory.tsx` at line 12, Guard against bot being
undefined by only calling createMineflayerConnector when bot is present and
return a safe fallback (e.g., null) otherwise; update the useMemo for connector
(the const connector = useMemo(...) line) to check the global bot before
invoking createMineflayerConnector and ensure connector is typed/handled for
null. Also stop relying solely on the global bot in the dependency array: wire
the Inventory component to a React-tracked source (e.g., local state or context
that holds the bot) or subscribe to bot changes in useEffect so re-renders occur
when a new bot is assigned, and use that tracked value in the useMemo dependency
instead of the raw global.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/inventoryWindows.ts (2)
49-84: Remove the large commented-out block inshowInventoryPlayer.This block now hides behavior intent and increases noise. Prefer deleting it (or moving rationale to a short comment / issue link).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/inventoryWindows.ts` around lines 49 - 84, Remove the large commented-out block inside showInventoryPlayer that contains the old openInventoryProxy.inventory/modelViewerState.model setup and the remotePlayerSkin loading logic; delete those commented lines (or replace them with a single short comment or an issue/PR link explaining why the code was removed) and keep any needed behavior implemented in the active code paths (references: showInventoryPlayer, openInventoryProxy.inventory, modelViewerState.model, remotePlayerSkin, loadSkinFromUsername, appViewer.playerState.reactive.playerSkin, lastWindowType).
28-28: Decouple proxy state from the UI component module.Importing
openInventoryProxyfromsrc/react/inventory/Inventory.tsxcouples this core file to the React/UI stack. Move proxy state to a small shared module (state-only) and import from both sides.♻️ Proposed refactor
- import { openInventoryProxy } from './react/inventory/Inventory' + import { openInventoryProxy } from './react/inventory/state'// src/react/inventory/state.ts import { proxy } from 'valtio' import type { InventoryOverlayProps } from 'minecraft-inventory/src' export const openInventoryProxy = proxy({ inventory: undefined as InventoryOverlayProps | undefined, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/inventoryWindows.ts` at line 28, Create a small shared state module that exports the Valtio proxy and stop importing openInventoryProxy from the React component module: move the proxy declaration into a new module (e.g., state.ts) that creates and exports openInventoryProxy = proxy({ inventory: undefined as InventoryOverlayProps | undefined }), update both the UI component (where openInventoryProxy was defined) and the core file that currently imports it to import openInventoryProxy from this new shared module, and remove the direct dependency on the React component (so symbols to change are openInventoryProxy, the proxy definition location, and the imports in the modules that currently reference it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react/inventory/Inventory.tsx`:
- Around line 1-4: The file is deep-importing internals (TextureProvider,
ScaleProvider, InventoryProvider, InventoryOverlay, createMineflayerConnector,
MineflayerBot, InventoryOverlayProps and localTexturesConfig) from
'minecraft-inventory/src' and 'minecraft-inventory/src/generated/localTextures';
replace those usages by either importing the same symbols from the package's
documented public entry (e.g., 'minecraft-inventory') or create a local adapter
module (e.g., src/lib/minecraftInventoryAdapter.ts) that re-exports only the
stable API your code needs and/or provides a local copy/wrapper for
localTexturesConfig; update Inventory.tsx to import from that public package or
your adapter so you no longer rely on private/internal paths.
- Around line 29-33: openPlayerInventory() sets openInventoryProxy.inventory = {
type: 'player' } but there is no cleanup to reset it when the modal closes;
update the modal close path (e.g., the hideCurrentModal() call or the close
callback used by InventoryOverlay) to clear openInventoryProxy.inventory =
undefined, and also ensure InventoryOverlay/unmount cleanup resets it (use a
component unmount handler or the onClose prop) so the proxy doesn't persist
after windowClose.
---
Nitpick comments:
In `@src/inventoryWindows.ts`:
- Around line 49-84: Remove the large commented-out block inside
showInventoryPlayer that contains the old
openInventoryProxy.inventory/modelViewerState.model setup and the
remotePlayerSkin loading logic; delete those commented lines (or replace them
with a single short comment or an issue/PR link explaining why the code was
removed) and keep any needed behavior implemented in the active code paths
(references: showInventoryPlayer, openInventoryProxy.inventory,
modelViewerState.model, remotePlayerSkin, loadSkinFromUsername,
appViewer.playerState.reactive.playerSkin, lastWindowType).
- Line 28: Create a small shared state module that exports the Valtio proxy and
stop importing openInventoryProxy from the React component module: move the
proxy declaration into a new module (e.g., state.ts) that creates and exports
openInventoryProxy = proxy({ inventory: undefined as InventoryOverlayProps |
undefined }), update both the UI component (where openInventoryProxy was
defined) and the core file that currently imports it to import
openInventoryProxy from this new shared module, and remove the direct dependency
on the React component (so symbols to change are openInventoryProxy, the proxy
definition location, and the imports in the modules that currently reference
it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a78e14d7-fd00-45ae-9f6a-7f804f369c79
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.jsonsrc/inventoryWindows.tssrc/react/inventory/Inventory.tsx
src/react/inventory/Inventory.tsx
Outdated
| import { TextureProvider, ScaleProvider, InventoryProvider, InventoryOverlay, createMineflayerConnector, MineflayerBot, InventoryOverlayProps } from 'minecraft-inventory/src' | ||
| import { useMemo } from 'react' | ||
| import { proxy, useSnapshot } from 'valtio' | ||
| import { localTexturesConfig } from 'minecraft-inventory/src/generated/localTextures' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For npm package minecraft-inventory@0.1.4, are minecraft-inventory/srcandminecraft-inventory/src/generated/localTextures documented public exports, or internal/private paths?
💡 Result:
The npm package minecraft-inventory@0.1.4 exists, as listed on libraries.io with a recent publication date (2026-03-14). It is described as "A flexible, scalable React library for rendering Minecraft inventory GUIs. Designed for integration into real Minecraft clients (e.g., ...". No official documentation, package.json exports, or README details the public API or mention minecraft-inventory/src or minecraft-inventory/src/generated/localTextures as documented exports. These paths appear to be internal/private source directories, typical for non-exported implementation files in npm packages (e.g., src folders are not public exports unless explicitly mapped in package.json "exports" or "files"). No sources indicate they are public or documented.
Citations:
Avoid deep-importing minecraft-inventory internals.
Lines 1 and 4 import from minecraft-inventory/src and minecraft-inventory/src/generated/localTextures, which are internal package paths with no documented public API. These imports couple the code to private implementation details and risk breaking on package updates. Use only public exports from the package, or wrap unstable dependencies in a local adapter module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react/inventory/Inventory.tsx` around lines 1 - 4, The file is
deep-importing internals (TextureProvider, ScaleProvider, InventoryProvider,
InventoryOverlay, createMineflayerConnector, MineflayerBot,
InventoryOverlayProps and localTexturesConfig) from 'minecraft-inventory/src'
and 'minecraft-inventory/src/generated/localTextures'; replace those usages by
either importing the same symbols from the package's documented public entry
(e.g., 'minecraft-inventory') or create a local adapter module (e.g.,
src/lib/minecraftInventoryAdapter.ts) that re-exports only the stable API your
code needs and/or provides a local copy/wrapper for localTexturesConfig; update
Inventory.tsx to import from that public package or your adapter so you no
longer rely on private/internal paths.
zardoy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/react/inventory/Inventory.tsx (2)
170-175:⚠️ Potential issue | 🟠 MajorGuard against
botbeing undefined.The
botglobal can beundefinedduring disconnect or before connection. Accessingbot as MineflayerBotandbot.versionwithout a guard will crash. This concern was raised in a previous review and the suggested fix pattern applies here.🛡️ Proposed fix
+ // Early return if bot is not available + if (!bot) return null + const connector = useMemo(() => { if (!inventoryType) return null - return createMineflayerConnector(bot as MineflayerBot, { + return createMineflayerConnector(bot, { itemMapper: buildItemMapper(bot.version), }) }, [textureVersion, !!inventoryType])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/inventory/Inventory.tsx` around lines 170 - 175, The connector useMemo must guard against bot being undefined to avoid crashes; inside the useMemo for connector (the createMineflayerConnector call in Inventory.tsx) return null immediately if !bot or !inventoryType, and only call createMineflayerConnector with bot (typed as MineflayerBot) and buildItemMapper(bot.version) when bot is defined; also update the dependency array to include bot (and keep textureVersion and inventoryType) so the memo recalculates when bot changes.
208-211:⚠️ Potential issue | 🟡 MinorAdd null check for
bot.currentWindow.While
bot.currentWindow?.close?.()uses optional chaining, accessingbotitself could fail if undefined. Consider guarding or ensuring this callback is only invoked whenbotis available.🛡️ Proposed fix
const handleClose = useCallback(() => { - if (bot.currentWindow) (bot.currentWindow as any).close?.() + bot?.currentWindow?.close?.() hideCurrentModal() }, [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/inventory/Inventory.tsx` around lines 208 - 211, The handleClose callback can throw if bot is undefined; update it to guard that bot exists before accessing bot.currentWindow (e.g., if (!bot) return), then safely call (bot.currentWindow as any)?.close?.(); also include bot and hideCurrentModal in the useCallback dependency array so the memoized handler stays correct; reference the handleClose function, bot.currentWindow access, and hideCurrentModal when making these changes.
🧹 Nitpick comments (3)
src/inventoryWindows.ts (1)
177-245: Consider removing commented-out legacy code.The commented-out legacy functions (
getAllItemRecipesLegacy,getAllItemUsagesLegacy) span 70+ lines. If these are no longer needed, removing them would improve code clarity. If they're kept for reference, consider moving to a separate file or documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/inventoryWindows.ts` around lines 177 - 245, Remove the large commented-out legacy blocks for getAllItemRecipesLegacy and getAllItemUsagesLegacy from src/inventoryWindows.ts (they start near the ingredientToItem helper) or move them into a dedicated legacy/archival file if you want to keep them for reference; ensure any referenced helper symbols used only by those blocks (e.g., ingredientToItem, PrismarineItem, mapSlots) remain used elsewhere or are cleaned up too, and run a quick compile/check after deletion to confirm no unused symbol errors.src/react/inventory/Inventory.tsx (1)
178-189: Duplicate event listeners withinventoryWindows.ts.Both this component (lines 183-184) and
inventoryWindows.ts(lines 117-118) register listeners forassetsInventoryReadyandassetsTexturesUpdatedthat callclearInventoryCaches(). This causesresetRenderedSlots()to be called multiple times per event.Consider centralizing the listener registration in one location to avoid redundant work.
♻️ Option: Remove listeners here and rely on inventoryWindows.ts
The listeners in
inventoryWindows.tsalready callclearInventoryCaches(). You could remove them here and just usetextureVersionbump via a different mechanism, or consolidate all refresh logic in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/inventory/Inventory.tsx` around lines 178 - 189, The Inventory component's useEffect in Inventory.tsx is registering the same 'assetsTexturesUpdated' and 'assetsInventoryReady' listeners that inventoryWindows.ts also registers, causing duplicate clearInventoryCaches() calls; remove the listener registration and teardown from Inventory.tsx's useEffect (the refresh callback and the appViewer.resourcesManager.on/off calls) and rely on the centralized registration in inventoryWindows.ts (or consolidate the refresh logic into a new shared function), and if Inventory still needs to update its textureVersion state, have inventoryWindows.ts call a shared updater or emit a single consolidated event so Inventory can bump setTextureVersion without causing duplicate clearInventoryCaches() invocations.src/resourcePack.ts (1)
568-580: Consider parallelizing texture override loading for faster resource pack application.The sequential
awaitin the loop works correctly but could be optimized for resource packs with many GUI textures. Additionally,resetRenderedSlots()is called here and also triggered by event listeners inInventory.tsx(viaclearInventoryCaches()), which may cause redundant re-renders during resource pack installation.♻️ Optional: Parallelize texture loading
// Apply resource-pack overrides for inventory GUI textures via the bundled config inventoryBundledConfig.clearOverrides() if (basePath) { - for (const texPath of allTexturePaths) { - const fsPath = `${basePath}/assets/minecraft/textures/${texPath}` - if (await existsAsync(fsPath)) { - const file = await fs.promises.readFile(fsPath, 'base64') - inventoryBundledConfig.setOverride(texPath, `data:image/png;base64,${file}`) - } - } + await Promise.all(allTexturePaths.map(async (texPath) => { + const fsPath = `${basePath}/assets/minecraft/textures/${texPath}` + if (await existsAsync(fsPath)) { + const file = await fs.promises.readFile(fsPath, 'base64') + inventoryBundledConfig.setOverride(texPath, `data:image/png;base64,${file}`) + } + })) } inventoryBundledConfig.resetRenderedSlots()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/resourcePack.ts` around lines 568 - 580, The current loop in resourcePack.ts sequentially awaits existsAsync and fs.promises.readFile for each path (allTexturePaths) which is slow for many textures and then calls inventoryBundledConfig.resetRenderedSlots(), duplicating resets already triggered by Inventory.tsx via clearInventoryCaches(); change to run existence checks and reads in parallel (e.g., map allTexturePaths to Promises and await Promise.all, then call inventoryBundledConfig.setOverride for successful results) and remove the immediate inventoryBundledConfig.resetRenderedSlots() here so the existing clearInventoryCaches()/Inventory.tsx listener handles rendering updates (or debounce/emit a single event) to avoid redundant re-renders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react/HotbarRenderApp.tsx`:
- Around line 238-239: HotbarInner is now dead code but still references removed
functions openItemsCanvas and upInventoryItems; either delete the HotbarInner
component entirely from HotbarRenderApp (and any exports/usages) if the hotbar
is deprecated, or update HotbarInner to call the new inventory API: replace
openItemsCanvas and upInventoryItems calls inside HotbarInner with the
corresponding new functions from the current inventory system (or prop
callbacks) and adjust imports/props accordingly so HotbarInner no longer
references nonexistent symbols; also re-enable the original return in
HotbarRenderApp only after these references are fixed.
---
Duplicate comments:
In `@src/react/inventory/Inventory.tsx`:
- Around line 170-175: The connector useMemo must guard against bot being
undefined to avoid crashes; inside the useMemo for connector (the
createMineflayerConnector call in Inventory.tsx) return null immediately if !bot
or !inventoryType, and only call createMineflayerConnector with bot (typed as
MineflayerBot) and buildItemMapper(bot.version) when bot is defined; also update
the dependency array to include bot (and keep textureVersion and inventoryType)
so the memo recalculates when bot changes.
- Around line 208-211: The handleClose callback can throw if bot is undefined;
update it to guard that bot exists before accessing bot.currentWindow (e.g., if
(!bot) return), then safely call (bot.currentWindow as any)?.close?.(); also
include bot and hideCurrentModal in the useCallback dependency array so the
memoized handler stays correct; reference the handleClose function,
bot.currentWindow access, and hideCurrentModal when making these changes.
---
Nitpick comments:
In `@src/inventoryWindows.ts`:
- Around line 177-245: Remove the large commented-out legacy blocks for
getAllItemRecipesLegacy and getAllItemUsagesLegacy from src/inventoryWindows.ts
(they start near the ingredientToItem helper) or move them into a dedicated
legacy/archival file if you want to keep them for reference; ensure any
referenced helper symbols used only by those blocks (e.g., ingredientToItem,
PrismarineItem, mapSlots) remain used elsewhere or are cleaned up too, and run a
quick compile/check after deletion to confirm no unused symbol errors.
In `@src/react/inventory/Inventory.tsx`:
- Around line 178-189: The Inventory component's useEffect in Inventory.tsx is
registering the same 'assetsTexturesUpdated' and 'assetsInventoryReady'
listeners that inventoryWindows.ts also registers, causing duplicate
clearInventoryCaches() calls; remove the listener registration and teardown from
Inventory.tsx's useEffect (the refresh callback and the
appViewer.resourcesManager.on/off calls) and rely on the centralized
registration in inventoryWindows.ts (or consolidate the refresh logic into a new
shared function), and if Inventory still needs to update its textureVersion
state, have inventoryWindows.ts call a shared updater or emit a single
consolidated event so Inventory can bump setTextureVersion without causing
duplicate clearInventoryCaches() invocations.
In `@src/resourcePack.ts`:
- Around line 568-580: The current loop in resourcePack.ts sequentially awaits
existsAsync and fs.promises.readFile for each path (allTexturePaths) which is
slow for many textures and then calls
inventoryBundledConfig.resetRenderedSlots(), duplicating resets already
triggered by Inventory.tsx via clearInventoryCaches(); change to run existence
checks and reads in parallel (e.g., map allTexturePaths to Promises and await
Promise.all, then call inventoryBundledConfig.setOverride for successful
results) and remove the immediate inventoryBundledConfig.resetRenderedSlots()
here so the existing clearInventoryCaches()/Inventory.tsx listener handles
rendering updates (or debounce/emit a single event) to avoid redundant
re-renders.
🪄 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: 8f64164a-fbe5-4eba-8187-0e01cb2b88f4
⛔ Files ignored due to path filters (6)
assets/generic_91.pngis excluded by!**/*.pngassets/generic_92.pngis excluded by!**/*.pngassets/generic_93.pngis excluded by!**/*.pngassets/generic_94.pngis excluded by!**/*.pngassets/generic_95.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
package.jsonsrc/inventoryWindows.tssrc/react/HotbarRenderApp.tsxsrc/react/inventory/Inventory.tsxsrc/react/inventory/inventoryTexturesConfig.tssrc/resourcePack.tssrc/resourcesSource.ts
💤 Files with no reviewable changes (1)
- src/resourcesSource.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
zardoy
changed the title
zardoy
changed the title
Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/react/inventory/Inventory.tsx (1)
75-81:⚠️ Potential issue | 🟡 MinorGuard against
botbeing undefined during disconnect.While the connector is only created when
inventoryTypeis truthy (which implies an active game session), there's a potential race condition if the player disconnects while the inventory modal is open. Thebotglobal can becomeundefinedduring disconnect.The risk is low since the modal would typically close before or during disconnect, but adding a defensive check would make the code more robust.
🛡️ Proposed defensive fix
const connector = useMemo(() => { - if (!inventoryType) return null + if (!inventoryType || !bot) return null return createMineflayerConnector(bot as MineflayerBot, { itemMapper: buildItemMapper(bot.version), formatTitle: formatWindowTitle, }) }, [textureVersion, !!inventoryType])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/inventory/Inventory.tsx` around lines 75 - 81, The connector creation in the useMemo for connector can run when bot becomes undefined during a disconnect; update the useMemo guard to also check that bot is defined before calling createMineflayerConnector so you only create the connector when inventoryType is truthy and bot (typed as MineflayerBot) is non-null; keep the same deps (textureVersion, !!inventoryType) but reference bot in the condition and avoid calling buildItemMapper or formatWindowTitle if bot is undefined to prevent runtime errors.
🧹 Nitpick comments (3)
src/react/inventory/sharedConnectorSetup.ts (1)
36-48: Consider adding error context to the catch block.The empty
catchblock silently swallows all errors during canvas extraction. While returningundefinedis a reasonable fallback, logging the error (at debug level) would help diagnose rendering issues when debugging.🔧 Optional: Add debug logging
try { const canvas = document.createElement('canvas') canvas.width = w canvas.height = h const ctx = canvas.getContext('2d')! ctx.drawImage(atlas, x, y, w, h, 0, 0, w, h) const url = canvas.toDataURL() spriteCache.set(cacheKey, url) return url - } catch { + } catch (err) { + console.debug('[sharedConnectorSetup] sprite extraction failed:', texture, slice, err) return undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/inventory/sharedConnectorSetup.ts` around lines 36 - 48, The catch block in the canvas extraction silently swallows errors—modify the try/catch in sharedConnectorSetup.ts (the block that creates a canvas, draws the atlas and returns url, referencing variables atlas, cacheKey, spriteCache) to catch the error as a variable and log it at debug level before returning undefined; include contextual info (cacheKey and sprite coordinates/dimensions x,y,w,h) in the debug message so failures are traceable (use the project logger if available, otherwise console.debug).src/react/HotbarRenderApp.tsx (2)
147-152: Minor inefficiency: MultipleclearInventoryCaches()subscriptions.Both
HotbarRenderAppandInventorysubscribe to the sameassetsTexturesUpdated/assetsInventoryReadyevents and callclearInventoryCaches(). Additionally,inventoryWindows.ts(lines 115-120) has a global subscription with a guard flag.When both components are mounted,
clearInventoryCaches()may be invoked 2-3 times per event. Since the operations are idempotent (clearing a Map and resetting state), this is functionally correct but slightly inefficient.Consider centralizing this subscription in
inventoryWindows.tsand using a reactive pattern (like a version counter in a shared state) that components can observe instead of each component independently subscribing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/HotbarRenderApp.tsx` around lines 147 - 152, HotbarRenderApp and Inventory both subscribe to appViewer.resourcesManager events and call clearInventoryCaches(), causing duplicate work; instead centralize the subscriptions in the existing inventoryWindows module: move the appViewer.resourcesManager.on('assetsTexturesUpdated'|'assetsInventoryReady', ...) handlers (the ones currently calling clearInventoryCaches and incrementing texture/version) into inventoryWindows.ts (use or extend its existing guard flag), expose a single shared reactive version counter or getter (e.g., inventoryTextureVersion or a subscribe/observable) that components (HotbarRenderApp uses setTextureVersion and Inventory) read or subscribe to; then remove the duplicate appViewer.resourcesManager.on(...) calls from HotbarRenderApp and Inventory so clearing happens once and components react to the shared version.
5-12: Deep imports fromminecraft-inventory/srcare consistent across the codebase, but acceptable for this private dependency.The imports access internal package paths (
/src/bundledTexturesConfig,/src/types,/src/registry,/src/components/JEI/JEI) rather than a documented public entry point. However, sinceminecraft-inventoryis maintained by the same author (zardoy) as this project and is not publicly published on npm, coordinated updates between the consumer and dependency mitigate the typical risk of deep imports breaking on package updates.The pattern is used consistently across multiple files (
Inventory.tsx,inventoryWindows.ts,sharedConnectorSetup.ts,resourcePack.ts), indicating an intentional architectural decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/HotbarRenderApp.tsx` around lines 5 - 12, Deep imports from 'minecraft-inventory/src' are used here (importing TextureProvider, ScaleProvider, InventoryProvider, InventoryWindow, createMineflayerConnector, MineflayerBot) and are intentional for this private dependency; add a short explanatory comment above these imports stating that deep imports are deliberate because 'minecraft-inventory' is a private, in-repo dependency maintained by the same author and may not expose a public entry point, and mention that any future public entry point should replace these deep imports when available; do not change the actual import lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react/inventory/sharedConnectorSetup.ts`:
- Around line 142-177: The null/undefined check in formatWindowTitle uses loose
equality (`rawTitle == null`); change it to an explicit check for both null and
undefined (i.e., test rawTitle === null || rawTitle === undefined) to satisfy
ESLint and make intent clear; update the guard at the top of formatWindowTitle
accordingly and leave all other logic untouched.
---
Duplicate comments:
In `@src/react/inventory/Inventory.tsx`:
- Around line 75-81: The connector creation in the useMemo for connector can run
when bot becomes undefined during a disconnect; update the useMemo guard to also
check that bot is defined before calling createMineflayerConnector so you only
create the connector when inventoryType is truthy and bot (typed as
MineflayerBot) is non-null; keep the same deps (textureVersion, !!inventoryType)
but reference bot in the condition and avoid calling buildItemMapper or
formatWindowTitle if bot is undefined to prevent runtime errors.
---
Nitpick comments:
In `@src/react/HotbarRenderApp.tsx`:
- Around line 147-152: HotbarRenderApp and Inventory both subscribe to
appViewer.resourcesManager events and call clearInventoryCaches(), causing
duplicate work; instead centralize the subscriptions in the existing
inventoryWindows module: move the
appViewer.resourcesManager.on('assetsTexturesUpdated'|'assetsInventoryReady',
...) handlers (the ones currently calling clearInventoryCaches and incrementing
texture/version) into inventoryWindows.ts (use or extend its existing guard
flag), expose a single shared reactive version counter or getter (e.g.,
inventoryTextureVersion or a subscribe/observable) that components
(HotbarRenderApp uses setTextureVersion and Inventory) read or subscribe to;
then remove the duplicate appViewer.resourcesManager.on(...) calls from
HotbarRenderApp and Inventory so clearing happens once and components react to
the shared version.
- Around line 5-12: Deep imports from 'minecraft-inventory/src' are used here
(importing TextureProvider, ScaleProvider, InventoryProvider, InventoryWindow,
createMineflayerConnector, MineflayerBot) and are intentional for this private
dependency; add a short explanatory comment above these imports stating that
deep imports are deliberate because 'minecraft-inventory' is a private, in-repo
dependency maintained by the same author and may not expose a public entry
point, and mention that any future public entry point should replace these deep
imports when available; do not change the actual import lines.
In `@src/react/inventory/sharedConnectorSetup.ts`:
- Around line 36-48: The catch block in the canvas extraction silently swallows
errors—modify the try/catch in sharedConnectorSetup.ts (the block that creates a
canvas, draws the atlas and returns url, referencing variables atlas, cacheKey,
spriteCache) to catch the error as a variable and log it at debug level before
returning undefined; include contextual info (cacheKey and sprite
coordinates/dimensions x,y,w,h) in the debug message so failures are traceable
(use the project logger if available, otherwise console.debug).
🪄 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: fd9b52b3-4a46-4a5e-9de2-e298f9e1f1bc
📒 Files selected for processing (4)
src/react/HotbarRenderApp.tsxsrc/react/OverlayModelViewer.tsxsrc/react/inventory/Inventory.tsxsrc/react/inventory/sharedConnectorSetup.ts
| export function formatWindowTitle (rawTitle: any): string { | ||
| if (rawTitle == null) return '' | ||
| if (typeof rawTitle === 'string') { | ||
| // Try to parse JSON text component | ||
| if (rawTitle.startsWith('{') || rawTitle.startsWith('"')) { | ||
| try { | ||
| return formatWindowTitle(JSON.parse(rawTitle)) | ||
| } catch { | ||
| // Not valid JSON — treat as plain text | ||
| } | ||
| } | ||
| return rawTitle | ||
| } | ||
| if (typeof rawTitle === 'object') { | ||
| // Handle NBT-simplified format: { value: "...", type: "string" } | ||
| if ('value' in rawTitle && rawTitle.type === 'string') { | ||
| return formatWindowTitle(rawTitle.value) | ||
| } | ||
| // Handle translate key: { translate: "container.chestDouble" } | ||
| if (rawTitle.translate) { | ||
| const lang = (globalThis as any).loadedData?.language | ||
| return lang?.[rawTitle.translate] ?? rawTitle.translate | ||
| } | ||
| // Handle text key: { text: "Custom Name" } | ||
| if (typeof rawTitle.text === 'string') { | ||
| return rawTitle.text | ||
| } | ||
| // Handle extra/with arrays by joining | ||
| if (rawTitle.extra) { | ||
| return (rawTitle.extra as any[]).map(formatWindowTitle).join('') | ||
| } | ||
| // Fallback: stringify for non-empty objects | ||
| if (typeof rawTitle[''] === 'string') return rawTitle[''] | ||
| } | ||
| return String(rawTitle) | ||
| } |
There was a problem hiding this comment.
Fix strict equality comparison per ESLint.
Line 143 uses == which ESLint flags. While == null is a common pattern to catch both null and undefined, changing to explicit checks satisfies the linter and makes intent clearer.
🔧 Proposed fix
export function formatWindowTitle (rawTitle: any): string {
- if (rawTitle == null) return ''
+ if (rawTitle === null || rawTitle === undefined) return ''
if (typeof rawTitle === 'string') {📝 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.
| export function formatWindowTitle (rawTitle: any): string { | |
| if (rawTitle == null) return '' | |
| if (typeof rawTitle === 'string') { | |
| // Try to parse JSON text component | |
| if (rawTitle.startsWith('{') || rawTitle.startsWith('"')) { | |
| try { | |
| return formatWindowTitle(JSON.parse(rawTitle)) | |
| } catch { | |
| // Not valid JSON — treat as plain text | |
| } | |
| } | |
| return rawTitle | |
| } | |
| if (typeof rawTitle === 'object') { | |
| // Handle NBT-simplified format: { value: "...", type: "string" } | |
| if ('value' in rawTitle && rawTitle.type === 'string') { | |
| return formatWindowTitle(rawTitle.value) | |
| } | |
| // Handle translate key: { translate: "container.chestDouble" } | |
| if (rawTitle.translate) { | |
| const lang = (globalThis as any).loadedData?.language | |
| return lang?.[rawTitle.translate] ?? rawTitle.translate | |
| } | |
| // Handle text key: { text: "Custom Name" } | |
| if (typeof rawTitle.text === 'string') { | |
| return rawTitle.text | |
| } | |
| // Handle extra/with arrays by joining | |
| if (rawTitle.extra) { | |
| return (rawTitle.extra as any[]).map(formatWindowTitle).join('') | |
| } | |
| // Fallback: stringify for non-empty objects | |
| if (typeof rawTitle[''] === 'string') return rawTitle[''] | |
| } | |
| return String(rawTitle) | |
| } | |
| export function formatWindowTitle (rawTitle: any): string { | |
| if (rawTitle === null || rawTitle === undefined) return '' | |
| if (typeof rawTitle === 'string') { | |
| // Try to parse JSON text component | |
| if (rawTitle.startsWith('{') || rawTitle.startsWith('"')) { | |
| try { | |
| return formatWindowTitle(JSON.parse(rawTitle)) | |
| } catch { | |
| // Not valid JSON — treat as plain text | |
| } | |
| } | |
| return rawTitle | |
| } | |
| if (typeof rawTitle === 'object') { | |
| // Handle NBT-simplified format: { value: "...", type: "string" } | |
| if ('value' in rawTitle && rawTitle.type === 'string') { | |
| return formatWindowTitle(rawTitle.value) | |
| } | |
| // Handle translate key: { translate: "container.chestDouble" } | |
| if (rawTitle.translate) { | |
| const lang = (globalThis as any).loadedData?.language | |
| return lang?.[rawTitle.translate] ?? rawTitle.translate | |
| } | |
| // Handle text key: { text: "Custom Name" } | |
| if (typeof rawTitle.text === 'string') { | |
| return rawTitle.text | |
| } | |
| // Handle extra/with arrays by joining | |
| if (rawTitle.extra) { | |
| return (rawTitle.extra as any[]).map(formatWindowTitle).join('') | |
| } | |
| // Fallback: stringify for non-empty objects | |
| if (typeof rawTitle[''] === 'string') return rawTitle[''] | |
| } | |
| return String(rawTitle) | |
| } |
🧰 Tools
🪛 ESLint
[error] 143-143: Use '===' to compare with null.
(no-eq-null)
[error] 143-143: Expected '===' and instead saw '=='.
(eqeqeq)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react/inventory/sharedConnectorSetup.ts` around lines 142 - 177, The
null/undefined check in formatWindowTitle uses loose equality (`rawTitle ==
null`); change it to an explicit check for both null and undefined (i.e., test
rawTitle === null || rawTitle === undefined) to satisfy ESLint and make intent
clear; update the guard at the top of formatWindowTitle accordingly and leave
all other logic untouched.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-e99qoinmo-zaro.vercel.app |
1 participant
A lot of code to be reviewed/finished and maybe simplified on integration part (here)
CHECKLIST:
Summary by CodeRabbit
New Features
Chores