big renderer codebase cleanup: clean player state #371
Conversation
use less dom things for better portabilitity / testing pick almost all changes update notification base
todo restore loadedData and hand
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis update performs a major refactor of the rendering, player state, and resource management systems. It introduces a fully reactive player state, splits renderer state into reactive and non-reactive components, modernizes texture and image handling for worker compatibility, and rearchitects resource management for transferability between threads. Numerous APIs, types, and initialization flows are updated to align with these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant MainThread as Main Thread
participant Worker as Worker Thread
MainThread->>ResourcesManager: prepareForTransfer()
ResourcesManager-->>MainThread: Transferable resources (ImageBitmaps, JSON)
MainThread->>Worker: postMessage(resources)
Worker->>ResourcesManager: restoreTransferred(data)
ResourcesManager-->>Worker: Rehydrate resources, set up event forwarding
MainThread->>PlayerState: getInitialPlayerState()
PlayerState-->>MainThread: Reactive state object
MainThread->>WorkerProxy: useWorkerProxy(method, args)
WorkerProxy->>Worker: postMessage({method, args, msgId})
Worker->>WorkerProxy: postMessage({result, msgId})
WorkerProxy-->>MainThread: Promise resolves with result
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (9)
src/browserfs.ts (1)
266-288:⚠️ Potential issueParameter leak & deprecated API
Recursive calls ignore the
removeDirectoryItselfflag – sub-directories are always removed, even when the caller asked to keep the root. Pass the flag explicitly if that behaviour is undesired.
fs.promises.rmdiris deprecated in Node 18+ (and upstream BrowserFS). Preferfs.promises.rm(path, { recursive: true })for forward compatibility.- await removeFileRecursiveAsync(curPath) + await removeFileRecursiveAsync(curPath, true) // keep intent explicit @@ -if (removeDirectoryItself) { - await fs.promises.rmdir(path) +if (removeDirectoryItself) { + await fs.promises.rm?.(path, { recursive: false }).catch(async () => { + // fallback for older BrowserFS + await fs.promises.rmdir(path) + }) }renderer/viewer/three/entity/EntityMesh.ts (1)
325-331:⚠️ Potential issuePotential
undefineddereference ofblocksTexture
blocksTexture!.clone()relies on the non-null assertion, butmaterial.mapcan legitimately beundefined
(e.g. if the caller never rendered world blocks first).
Add an early exit or throw with a clear message.-if (textureOffset) { - // todo(memory) dont clone - const loadedTexture = blocksTexture!.clone() +if (textureOffset) { + if (!blocksTexture) { + console.error('Block atlas texture not available yet – cannot apply offset') + } else { + // todo(memory) dont clone + const loadedTexture = blocksTexture.clone() loadedTexture.offset.set(textureOffset[0], textureOffset[1]) loadedTexture.needsUpdate = true material.map = loadedTexture + }renderer/viewer/three/holdingBlock.ts (1)
345-358: 🛠️ Refactor suggestion
itemUsageTicksmay beundefinedThe mesh call expects a number; when the player is not using an item you pass
undefined, producingNaNinside math later on. Default to 0.src/mineflayer/items.ts (1)
114-131: 🛠️ Refactor suggestion
⚠️ Potential issue
resourcesManagerparameter is ignored – leads to hidden coupling.
getItemModelNamereceives aresourcesManagerbut later readsappViewer.resourcesManager, silently discarding the caller-supplied argument. This breaks testability and makes the function depend on a global.- const modelFromDef = getItemDefinition(appViewer.resourcesManager.itemsDefinitionsStore, { + const modelFromDef = getItemDefinition(resourcesManager.itemsDefinitionsStore, { name: itemModelName, - version: appViewer.resourcesManager.currentResources!.version, + version: resourcesManager.currentResources!.version, properties: itemSelector })?.modelAlso note that
playerStateis currently passed togetItemSelectorbut the selector helper does not read it – either make use of it or remove the extra param to keep the API clean.renderer/viewer/three/entities.ts (2)
597-606:⚠️ Potential issueDefensive check before non-null
worldBlockProvider!.
currentResourcescan beundefinedduring early loading → accessingworldBlockProvider!crashes.- const provider = this.worldRenderer.resourcesManager.currentResources.worldBlockProvider! - const mesh = getBlockMeshFromModel(this.worldRenderer.material, textureUv.resolvedModel, textureUv.modelName, provider) + const provider = this.worldRenderer.resourcesManager.currentResources?.worldBlockProvider + if (!provider) return + const mesh = getBlockMeshFromModel(this.worldRenderer.material, textureUv.resolvedModel, textureUv.modelName, provider)Keeps the render loop alive while resources load.
99-129: 💡 Verification agent❓ Verification inconclusive
OffscreenCanvas+ Three.js texture – type / runtime compatibility.
new THREE.Texture(canvas)expectsHTMLCanvasElement | ImageBitmap.
OffscreenCanvasis accepted only in very recent Three.js versions and is not in
the public type union, causing TS errors in strict mode and crashing on older browsers.Safest cross-browser route:
- const canvas = new OffscreenCanvas(64, 64) + const offCanvas = new OffscreenCanvas(64, 64) + const canvas = document.createElement('canvas') + canvas.width = offCanvas.width + canvas.height = offCanvas.height + canvas.getContext('2d')!.drawImage(offCanvas, 0, 0)(or
offCanvas.transferToImageBitmap()and feed anImageBitmap).Consider gating the OffscreenCanvas path with feature detection to avoid regressions.
Ensure OffscreenCanvas compatibility when creating Three.js textures
Three.js’sTextureconstructor accepts onlyHTMLCanvasElement,ImageBitmap, etc.—OffscreenCanvas isn’t part of the public type union and will trigger TS errors in strict mode and fail in browsers that lack full OffscreenCanvas support.Use one of these safe cross-browser approaches:
• Option 1: Draw the OffscreenCanvas into an HTMLCanvasElement
- const canvas = new OffscreenCanvas(64, 64) + const offscreen = new OffscreenCanvas(64, 64) + const ctxOff = offscreen.getContext('2d')! + // … draw onto offscreen … + const canvas = document.createElement('canvas') + canvas.width = offscreen.width + canvas.height = offscreen.height + canvas.getContext('2d')!.drawImage(offscreen, 0, 0) // then: const texture = new THREE.Texture(canvas)• Option 2: Transfer to an ImageBitmap and use that directly
const offscreen = new OffscreenCanvas(64, 64) const ctx = offscreen.getContext('2d')! // … draw onto offscreen … const bitmap = offscreen.transferToImageBitmap() const texture = new THREE.Texture(bitmap)Wrap either approach in feature detection:
if (typeof OffscreenCanvas !== 'undefined') { // use offscreen path… } else { // create and draw directly into HTMLCanvasElement… }This avoids TS type errors and runtime crashes on browsers without full OffscreenCanvas support.
renderer/viewer/lib/worldrendererCommon.ts (3)
238-248:⚠️ Potential issueRace condition between worker init and asset push
init()firesPromise.all([ this.resetWorkers(), updateAssetsData ]), letting
updateAssetsData()run beforeresetWorkers()finishes.
If that happens, the first statement inupdateAssetsDatathrows
“workers not initialized yet”.Make the calls sequential:
- await Promise.all([ this.resetWorkers(), this.updateAssetsDataIfReady() ]) +await this.resetWorkers(); +await this.updateAssetsDataIfReady();(or chain one after the other).
591-606: 🛠️ Refactor suggestionGuard against
resourcesManager.currentResourcesbeing undefined
updateAssetsData()assumescurrentResourcesis present, yet it can be
undefinedwhen the manager is still loading. Add an early return to avoid a
crash:const resources = this.resourcesManager.currentResources; if (!resources) return; // still loading
783-792:⚠️ Potential issueDuplicate
renderDistancelistenersThe same
worldEmitter.on('renderDistance', …)block is registered twice,
executing the body twice for every update and skewingchunksLength,
viewDistance, andallChunksFinished.Delete one of the handlers or consolidate the logic.
🧹 Nitpick comments (20)
renderer/viewer/lib/mesher/shared.ts (1)
12-12: Remove or gatekeep commented-out configuration for long-term cleanlinessLeaving
textureSizecommented out risks it silently diverging from the authoritative defaultMesherConfig definition over time. Either delete the line altogether or gate it behind a debug flag so its intent is explicit.src/react/RendererDebugMenu.tsx (2)
11-13: Default export never re-renders ifwindow.worldappears later
const worldRenderer = window.world as WorldRendererCommon | undefinedis read once when the component mounts.
Ifwindow.worldis attached asynchronously (e.g. after initial handshake) the debug menu will never mount.Consider subscribing to a reactive source (e.g. a proxy or context) or, simpler:
-const worldRenderer = window.world as WorldRendererCommon | undefined -return worldRenderer ? <RendererDebugMenu worldRenderer={worldRenderer} /> : null +const [, forceUpdate] = useState(0) +useEffect(() => { + const id = setInterval(() => { + if (window.world) forceUpdate(i => i + 1) + }, 100) + return () => clearInterval(id) +}, []) +return window.world ? <RendererDebugMenu worldRenderer={window.world as WorldRendererCommon} /> : nullThis keeps the component dormant when unnecessary yet reacts once the global is ready.
15-15: Name the component for clearer stack tracesAnonymous arrow functions show up as “Unknown” in React DevTools and error stacks. Give the constant an explicit name:
-const RendererDebugMenu = ({ worldRenderer }: { worldRenderer: WorldRendererCommon }) => { +const RendererDebugMenu: React.FC<{ worldRenderer: WorldRendererCommon }> = ({ worldRenderer }) => {renderer/viewer/three/world/cursorBlock.ts (1)
104-105: Potentialundefinedtexture accessEven with the fallback
?? this.breakTextures.at(-1)the array can still be empty (first render before any image arrives).
Provide a 1×1 transparent placeholder to guaranteematerial.mapis always a validTHREE.Texture.renderer/viewer/lib/utils.ts (1)
7-21: Cache initialisation raceIf two concurrent calls request the same texture before the first finishes, both will enter the
!cachedbranch and overwriteimagesPromises[texture], losing one resolver.
Wrap the!cachedblock withif (!imagesPromises[texture])or use aMapkeyed by URL/promise to guarantee single flight.renderer/viewer/three/graphicsBackend.ts (1)
60-70: Global mutation ofResourcesManager.currentConfigOverwriting
currentConfigfor thedebugEntitiesroute mutates the shared singleton and may leak into subsequent world loads.
Consider cloning a freshResourcesManagerinstance or restoring the previous config after the debug view completes.renderer/viewer/baseGraphicsBackend.ts (1)
8-24: DuplicatedchunksLoadedsets may desynchronise
reactive.world.chunksLoadedandnonReactive.world.chunksLoadedare created as two distinctSetinstances.
Code that mutates one will not affect the other, leading to inconsistent progress reporting.Consider sharing the same reference:
- reactive: { - world: { chunksLoaded: new Set(), … }, - }, - nonReactive: { - world: { chunksLoaded: new Set(), … }, - } + const chunksLoaded = new Set<string>() + … + reactive: { world: { chunksLoaded, … }, … }, + nonReactive: { world: { chunksLoaded, … }, … },renderer/viewer/three/appShared.ts (1)
13-14: Remove commented-outtexturepropertyThese commented lines add noise and may confuse future readers about the intended API surface.
- // texture: ImageBitmap … - // texture: img, … - // texture: resources.blocksAtlasImage,Also applies to: 56-57, 70-71
src/index.ts (1)
737-740: HeavyupdateAssetsDataruns on every world load
updateAssetsData({})is expensive and is now executed every timedisplayWorldruns, even if assets are already ready.
Add a flag or rely onresourcesManager.promiseAssetsReadyinstead.src/react/Notification.tsx (1)
41-64:top === 0test misses touch-devicesWhen
topPosition === 0and the user is on a touch device,topbecomes 18, so the very first toast renders with rounded top-left corners, unlike the desktop case.
If the intention is “first toast has square corner regardless of device”, comparetopPositioninstead of the computed pixel value.renderer/viewer/lib/utils/skins.ts (1)
6-17:loadThreeJsTextureFromUrlSyncnever disposes its TextureLoaderCreating a fresh
THREE.Texture()is fine, but spinning up aTextureLoaderper call is wasteful.
Reuse a module-level loader or callloadThreeJsTextureFromUrl.renderer/viewer/lib/workerProxy.ts (1)
3-3: Confusingvoid | Promiseunion
voidinside the union makes the handler appear synchronous but you never post a result for that path. Either always return a promise or useunknownto avoid misleading “void”.🧰 Tools
🪛 Biome (1.9.4)
[error] 3-3: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/mineflayer/items.ts (1)
7-7: Drop unused imports.
ResourcesManagerandResourcesManagerTransferredare not referenced anymore – keep the diff minimal and avoid future lint noise by removing them.src/resourcePack.ts (2)
520-531: Deeply nested promise + mixed indent harms readability.The 700 ms delay block mixes spaces and collapses logic:
if (Date.now() - start < 700) { void new Promise(res => setTimeout(res, 700)).then(() => { if (choice === false || choice === 'Pretend Installed (not recommended)') { bot.denyResourcePack() } else { bot.acceptResourcePack() } }) }• Fixes ESLint
indentnoise.
• Makes the accept/deny condition explicit.🧰 Tools
🪛 ESLint
[error] 525-525: Expected indentation of 12 spaces but found 6.
(@stylistic/indent)
[error] 526-526: Expected indentation of 12 spaces but found 6.
(@stylistic/indent)
595-604: Stage name mismatched with parallel workload.
beginStage('generate-atlas-texture-blocks', …)launches four concurrent tasks (blocks, items, armor, UI).
Rename to something generic (generate-atlas-textures) or create subtasks for clearer progress reporting.renderer/viewer/lib/basePlayerState.ts (1)
61-67: Dead parameter –playerStateis unused.
getItemSelectorsignature acceptsplayerStatebut the body never references it.
Either incorporate reactive state (e.g., hand, sneaking flags) or drop the parameter to avoid misleading APIs.src/mineflayer/playerState.ts (1)
100-103:OFF_GROUND_THRESHOLDis zero – movement state jumps to SNEAKING immediatelyWith
OFF_GROUND_THRESHOLD = 0, one physics tick in the air is enough to classify the player asSNEAKING, which feels unintended (was likely a larger debounce previously).Consider restoring a small buffer (e.g., 150 ms) or using the vanilla “airborne” state directly.
Also applies to: 115-117
src/resourcesManager.ts (2)
50-55: Avoiddelete– assignundefinedinsteadUsing
deleteon class fields (itemsRenderer,worldBlockProvider) de-optimises hidden-class objects in V8 and hurts performance.- delete this.itemsRenderer - delete this.worldBlockProvider + this.itemsRenderer = undefined + this.worldBlockProvider = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
82-101: Rename the innerdataparameter to avoid shadowingThe callback passed to
addEventListener('message', ({ data }) => { … })shadows the outerdataargument ofrestoreTransferred, making the code harder to read and easy to misuse. Pick a different name, e.g.msg.renderer/viewer/lib/worldrendererCommon.ts (1)
1028-1046:process.envreference breaks in browsers without bundler shims
initMesherWorkerrelies onprocess.env.SINGLE_FILE_BUILD, which is undefined
in vanilla browsers and triggers a ReferenceError when the code is served
unbundled. Gate the check:const singleFile = typeof process !== 'undefined' && process.env?.SINGLE_FILE_BUILD;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.gitignore(1 hunks)renderer/buildMesherWorker.mjs(1 hunks)renderer/viewer/baseGraphicsBackend.ts(1 hunks)renderer/viewer/lib/basePlayerState.ts(1 hunks)renderer/viewer/lib/guiRenderer.ts(4 hunks)renderer/viewer/lib/mesher/mesher.ts(2 hunks)renderer/viewer/lib/mesher/shared.ts(1 hunks)renderer/viewer/lib/utils.ts(2 hunks)renderer/viewer/lib/utils/skins.ts(2 hunks)renderer/viewer/lib/workerProxy.ts(3 hunks)renderer/viewer/lib/worldDataEmitter.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(17 hunks)renderer/viewer/three/appShared.ts(4 hunks)renderer/viewer/three/documentRenderer.ts(9 hunks)renderer/viewer/three/entities.ts(13 hunks)renderer/viewer/three/entity/EntityMesh.ts(2 hunks)renderer/viewer/three/graphicsBackend.ts(2 hunks)renderer/viewer/three/holdingBlock.ts(7 hunks)renderer/viewer/three/panorama.ts(5 hunks)renderer/viewer/three/renderSlot.ts(1 hunks)renderer/viewer/three/world/cursorBlock.ts(2 hunks)renderer/viewer/three/worldrendererThree.ts(10 hunks)src/appViewer.ts(9 hunks)src/browserfs.ts(2 hunks)src/index.ts(8 hunks)src/inventoryWindows.ts(5 hunks)src/mineflayer/items.ts(4 hunks)src/mineflayer/playerState.ts(6 hunks)src/react/HotbarRenderApp.tsx(1 hunks)src/react/Notification.tsx(2 hunks)src/react/RendererDebugMenu.tsx(1 hunks)src/rendererUtils.ts(2 hunks)src/resourcePack.ts(6 hunks)src/resourcesManager.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
renderer/viewer/lib/utils.ts (1)
renderer/viewer/lib/utils/skins.ts (1)
loadThreeJsTextureFromUrlSync(6-17)
renderer/viewer/three/graphicsBackend.ts (4)
renderer/viewer/three/primitives.js (1)
THREE(3-3)src/resourcesManager.ts (1)
ResourcesManager(79-300)renderer/playground/allEntitiesDebug.ts (1)
displayEntitiesDebugList(3-170)renderer/viewer/three/panorama.ts (1)
PanoramaRenderer(26-245)
src/react/Notification.tsx (1)
src/react/utilsApp.ts (1)
useUsingTouch(17-19)
src/rendererUtils.ts (1)
src/mineflayer/playerState.ts (1)
playerState(174-174)
renderer/viewer/baseGraphicsBackend.ts (1)
src/appViewer.ts (2)
RendererReactiveState(19-30)NonReactiveState(31-36)
src/inventoryWindows.ts (4)
src/appViewer.ts (1)
appViewer(275-275)src/mineflayer/items.ts (1)
getItemModelName(114-131)src/mineflayer/playerState.ts (1)
playerState(174-174)renderer/viewer/three/renderSlot.ts (1)
renderSlot(11-78)
renderer/viewer/lib/utils/skins.ts (2)
renderer/viewer/three/primitives.js (1)
THREE(3-3)renderer/viewer/lib/guiRenderer.ts (1)
getLoadedImage(248-252)
src/mineflayer/items.ts (3)
src/resourcesManager.ts (1)
ResourcesManagerCommon(74-76)renderer/viewer/lib/basePlayerState.ts (3)
ItemSpecificContextProperties(7-7)PlayerStateRenderer(57-59)getItemSelector(61-68)src/mineflayer/playerState.ts (1)
playerState(174-174)
renderer/viewer/three/entities.ts (5)
renderer/viewer/three/primitives.js (1)
THREE(3-3)renderer/viewer/lib/utils/skins.ts (2)
stevePngUrl(24-24)steveTexture(25-25)renderer/viewer/three/entity/animations.js (1)
WalkingGeneralSwing(4-108)renderer/viewer/three/holdingBlock.ts (1)
getBlockMeshFromModel(925-931)renderer/viewer/three/entity/armorModels.ts (1)
armorTextures(19-35)
🪛 Biome (1.9.4)
renderer/viewer/lib/workerProxy.ts
[error] 3-3: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 70-70: Do not add then to an object.
(lint/suspicious/noThenProperty)
src/resourcesManager.ts
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 ESLint
renderer/viewer/three/renderSlot.ts
[error] 57-57: Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong.
(@typescript-eslint/no-non-null-asserted-optional-chain)
[error] 60-60: Optional chain expressions can return undefined by design - using a non-null assertion is unsafe and wrong.
(@typescript-eslint/no-non-null-asserted-optional-chain)
src/resourcePack.ts
[error] 525-525: Expected indentation of 12 spaces but found 6.
(@stylistic/indent)
[error] 526-526: Expected indentation of 12 spaces but found 6.
(@stylistic/indent)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (9)
.gitignore (1)
22-22: Approve ignoring logs directoryAdding
logs/ensures that runtime log files aren't tracked in Git.renderer/buildMesherWorker.mjs (1)
38-41: Asset loaders added – looks correctNice addition;
.png → dataurland.obj → textmatch esbuild’s supported loaders. Confirm if other binary assets (e.g..jpg,.ktx2) are ever referenced—if so, extend the map now to avoid runtime 404s later.src/react/HotbarRenderApp.tsx (1)
115-116: Good simplification of atlas-parser guardSwitching from the nested
currentResources?.itemsAtlasParsertoresourcesManager?.itemsAtlasParserreflects the new API and removes an indirection layer—LGTM.src/rendererUtils.ts (1)
15-17:❌ Incorrect review comment
botmay beundefinedafter readiness checkThe guard switched from
if (!bot) returntoif (!playerState.ready) return, yet the function still dereferencesbot.*farther down.
playerState.ready === truedoes not guarantee the globalbotobject is initialised, leading to runtime crashes.Restore the original guard or chain both:
-if (!playerState.ready) return +if (!playerState.ready || !bot) returnPlease verify initialisation order in single-/multi-player branches.
🏁 Script executed:
#!/bin/bash # Locate the rendererUtils file file=$(fd rendererUtils.ts | head -n 1) echo "Inspecting $file" # Show the first 200 lines to get context sed -n '1,200p' "$file" echo "---" # Show the updateFovAnimation function and surrounding lines rg -n "updateFovAnimation" -A10 "$file" echo "---" # Show all bot property accesses in this file rg -n "bot\." -n "$file"Length of output: 4433
🏁 Script executed:
#!/bin/bash # Locate playerState file ps_file=$(fd playerState.ts | head -n 1) echo "Inspecting $ps_file" sed -n '1,200p' "$ps_file" echo "--- All occurrences of 'ready' in $ps_file ---" rg -n "ready" -n "$ps_file" echo "--- Search for 'bot' declaration in the repo ---" rg -n "let bot" -n . rg -n "const bot" -n . rg -n "export.*bot" -n . rg -n "bot =" -n .Length of output: 34176
🏁 Script executed:
#!/bin/bash # Show context around bot creation in src/index.ts echo "===== src/index.ts: Lines 450-600 =====" sed -n '450,600p' src/index.ts echo # Search for where mineflayerBotCreated is emitted echo "===== customEvents.emit occurrences in src/index.ts =====" rg -n "customEvents.emit" -n src/index.tsLength of output: 6379
Ignore redundant
botcheck inupdateFovAnimationThe
playerState.readyflag only flips totrueafter the globalbotinstance has been created and itsinject_allowedevent has fired, and allbotreferences insideupdateFovAnimationalready use optional chaining for safety. Adding|| !botwould be redundant here. No changes required.Likely an incorrect or invalid review comment.
src/react/Notification.tsx (1)
61-66: Z-index inversionLoaders (
type === 'progress') receivezIndex = 10while regular toasts get 1200, causing them to hide underneath any overlapping UI. Reverse the values if the loader should stay on top.renderer/viewer/lib/utils/skins.ts (1)
71-78: Verify OffscreenCanvas compatibility
loadSkinToCanvasfromskinview-utilshistorically expects anHTMLCanvasElement. Confirm it copes withOffscreenCanvas; otherwise fall back to a normal canvas on main thread.renderer/viewer/three/documentRenderer.ts (1)
125-130: Avoid ultra-tight render loop whentimeoutRenderingis enabledIf
config.timeoutRenderingistrueandfpsLimitis falsy,setTimeout(animate, 0)re-queues immediately, pegging the CPU.Guard with a sane default:
-const delay = this.config.fpsLimit ? 1000 / this.config.fpsLimit : 0 +const delay = this.config.fpsLimit ? 1000 / this.config.fpsLimit : 16 // ~60 FPS fallback this.timeoutId = setTimeout(animate, delay) as unknown as numbersrc/resourcesManager.ts (1)
104-125: Overridingemitleaks cross-thread listeners
prepareForTransfermonkey-patchesthis.emitto forward every event to the worker.
There is no corresponding “unpatch” when the manager is disposed, so the patched
function – and the capturedworkerreference – stay alive forever, leading to
memory leaks on hot reload / multiple transfers.Expose a dedicated forwarding helper (e.g.
forwardEvents(worker) { … return () => unhook() })
and call its teardown indestroy().src/appViewer.ts (1)
168-170: Null-safety: guardworldView/botbefore dereferencing
this.worldView!.init(bot.entity.position)assumes both globals exist. In
headless or menu statesbotorworldViewmay be undefined, raising an
exception that stops backend initialisation.
Defensive check recommended.
| for (let i = 0; i < 10; i++) { | ||
| const texture = loader.load(destroyStagesImages[i]) | ||
| texture.magFilter = THREE.NearestFilter | ||
| texture.minFilter = THREE.NearestFilter | ||
| this.breakTextures.push(texture) | ||
| void loadThreeJsTextureFromUrl(destroyStagesImages[i]).then((texture) => { | ||
| texture.magFilter = THREE.NearestFilter | ||
| texture.minFilter = THREE.NearestFilter | ||
| this.breakTextures.push(texture) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Race condition & wrong texture-stage mapping
Array.push stores textures in the order promises resolve, not in the i index order.
If any image loads out-of-order (very likely over the network) breakTextures[stage] will point to the wrong texture, causing flickering or wrong destroy-stage overlays.
-void loadThreeJsTextureFromUrl(destroyStagesImages[i]).then((texture) => {
- texture.magFilter = THREE.NearestFilter
- texture.minFilter = THREE.NearestFilter
- this.breakTextures.push(texture)
-})
+// preserve stage → texture mapping
+void loadThreeJsTextureFromUrl(destroyStagesImages[i]).then((texture) => {
+ texture.magFilter = THREE.NearestFilter
+ texture.minFilter = THREE.NearestFilter
+ this.breakTextures[i] = texture // keep index stable
+})Also consider pre-allocating this.breakTextures.length = 10 so updateBreakAnimation never dereferences undefined.
📝 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.
| for (let i = 0; i < 10; i++) { | |
| const texture = loader.load(destroyStagesImages[i]) | |
| texture.magFilter = THREE.NearestFilter | |
| texture.minFilter = THREE.NearestFilter | |
| this.breakTextures.push(texture) | |
| void loadThreeJsTextureFromUrl(destroyStagesImages[i]).then((texture) => { | |
| texture.magFilter = THREE.NearestFilter | |
| texture.minFilter = THREE.NearestFilter | |
| this.breakTextures.push(texture) | |
| }) | |
| for (let i = 0; i < 10; i++) { | |
| // preserve stage → texture mapping | |
| void loadThreeJsTextureFromUrl(destroyStagesImages[i]).then((texture) => { | |
| texture.magFilter = THREE.NearestFilter | |
| texture.minFilter = THREE.NearestFilter | |
| this.breakTextures[i] = texture // keep index stable | |
| }) | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/three/world/cursorBlock.ts around lines 45 to 50, the code
pushes textures into this.breakTextures as promises resolve, causing a race
condition and incorrect mapping between textures and their destroy stages. To
fix this, pre-allocate this.breakTextures with length 10 and assign each loaded
texture directly to the correct index based on i instead of pushing. This
ensures the textures are stored in the correct order and prevents undefined
entries during updateBreakAnimation.
| this.camera = new THREE.PerspectiveCamera(85, this.documentRenderer.canvas.width / this.documentRenderer.canvas.height, 0.05, 1000) | ||
| this.camera.position.set(0, 0, 0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Derive aspect ratio from the renderer, not the canvas element
HTMLCanvasElement.width/height can differ from the display size (device-pixel-ratio, OffscreenCanvas, manual resizes).
Use the renderer’s measured size to stay correct:
- this.camera = new THREE.PerspectiveCamera(85, this.documentRenderer.canvas.width / this.documentRenderer.canvas.height, 0.05, 1000)
+const size = this.documentRenderer.renderer.getSize(new THREE.Vector2())
+this.camera = new THREE.PerspectiveCamera(85, size.x / size.y, 0.05, 1000)and inside the resize handler:
- this.camera.aspect = this.documentRenderer.canvas.width / this.documentRenderer.canvas.height
+const newSize = this.documentRenderer.renderer.getSize(new THREE.Vector2())
+this.camera.aspect = newSize.x / newSize.yAlso applies to: 68-70
🤖 Prompt for AI Agents
In renderer/viewer/three/panorama.ts around lines 53 to 54 and also lines 68 to
70, the aspect ratio for the PerspectiveCamera is currently derived from the
canvas element's width and height, which may not reflect the actual renderer
size due to device pixel ratio or resizing. To fix this, obtain the renderer's
size using its getSize method or equivalent, and use those dimensions to
calculate the aspect ratio. Also, update the camera's aspect ratio and call
updateProjectionMatrix inside the resize handler to keep the camera projection
correct on window or canvas size changes.
| async debugImageInFrontOfCamera () { | ||
| const image = await loadThreeJsTextureFromUrl(join('background', 'panorama_0.png')) | ||
| const mesh = new THREE.Mesh(new THREE.PlaneGeometry(1000, 1000), new THREE.MeshBasicMaterial({ map: image })) | ||
| mesh.position.set(0, 0, -500) | ||
| mesh.rotation.set(0, 0, 0) | ||
| this.scene.add(mesh) | ||
| } |
There was a problem hiding this comment.
Incorrect texture object passed to material
loadThreeJsTextureFromUrl returns { texture }, yet the code feeds the wrapper object into MeshBasicMaterial.map.
-const image = await loadThreeJsTextureFromUrl(...)
-const mesh = new THREE.Mesh(new THREE.PlaneGeometry(1000, 1000),
- new THREE.MeshBasicMaterial({ map: image }))
+const { texture } = await loadThreeJsTextureFromUrl(...)
+const mesh = new THREE.Mesh(new THREE.PlaneGeometry(1000, 1000),
+ new THREE.MeshBasicMaterial({ map: texture }))Without the fix the plane renders black.
📝 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.
| async debugImageInFrontOfCamera () { | |
| const image = await loadThreeJsTextureFromUrl(join('background', 'panorama_0.png')) | |
| const mesh = new THREE.Mesh(new THREE.PlaneGeometry(1000, 1000), new THREE.MeshBasicMaterial({ map: image })) | |
| mesh.position.set(0, 0, -500) | |
| mesh.rotation.set(0, 0, 0) | |
| this.scene.add(mesh) | |
| } | |
| async debugImageInFrontOfCamera () { | |
| const { texture } = await loadThreeJsTextureFromUrl(join('background', 'panorama_0.png')) | |
| const mesh = new THREE.Mesh(new THREE.PlaneGeometry(1000, 1000), new THREE.MeshBasicMaterial({ map: texture })) | |
| mesh.position.set(0, 0, -500) | |
| mesh.rotation.set(0, 0, 0) | |
| this.scene.add(mesh) | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/three/panorama.ts around lines 75 to 81, the code incorrectly
passes the entire object returned by loadThreeJsTextureFromUrl to
MeshBasicMaterial.map instead of the actual texture property. To fix this,
extract the texture property from the returned object and pass that texture to
the map parameter so the material receives the correct texture and the plane
renders properly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-1rms50tt2-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/resourcesManager.ts (1)
108-116:⚠️ Potential issue
updateAssetsDatacreates a localAbortController.
destroy()abortsthis.abortController, not the one instantiated here, so long-running atlas generation keeps running afterdestroy()is called.- const abortController = new AbortController() + this.abortController = new AbortController() + const abortController = this.abortControllerWithout this, cancelling a hot reload may leak CPU & memory.
🧹 Nitpick comments (3)
src/resourcesManager.ts (3)
24-41: Unify texture types underImageBitmap.
customTextures.*.texturesstill holdHTMLImageElement, which defeats the transfer-friendly objective of this refactor. Consider storingImageBitmap(or at least allowing both) so the whole structure can be cloned/posted without hitting the structured-clone HTML element blacklist.
173-209: Atlas parser recreated just fortoDataURL.
new AtlasParser({ latest: blocksAtlas }, blocksCanvas.toDataURL())pays the full PNG encoding cost only to build a parser. Since you already keepblocksCanvas, consider acceptingCanvasImageSource | stringin the parser or providing it the existing binary instead of forcing a data URL conversion.
211-228:recreateItemsAtlasmirrors block logic – maybe deduplicate.Both atlas builders share 90 % of the flow; extracting a generic helper would reduce duplication and future drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
renderer/viewer/lib/worldDataEmitter.ts(3 hunks)renderer/viewer/three/holdingBlock.ts(7 hunks)src/appViewer.ts(9 hunks)src/index.ts(10 hunks)src/resourcePack.ts(6 hunks)src/resourcesManager.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- renderer/viewer/three/holdingBlock.ts
- src/index.ts
- src/resourcePack.ts
- renderer/viewer/lib/worldDataEmitter.ts
- src/appViewer.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/resourcesManager.ts
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (8)
src/resourcesManager.ts (8)
11-11: Import looks good.Adding
ItemsAtlasesOutputJsonis appropriate for the newblocksAtlasJsonfield.
20-20: 👍 NewassetsInventoryStartedevent.The extra event will make it easier for consumers to hook into the inventory build pipeline.
71-76: Interface naming is clear.Splitting Transferred vs Common clarifies the runtime transport boundary.
80-80: Nicely exposesrestorerName.Matches the restoration pattern used elsewhere.
89-91: Potential undefined access to atlas parsers.
itemsAtlasParser/blocksAtlasParserare declared non-optional, yet they’re assigned later. Any call todownloadDebugAtlas()(or other future helpers) made beforeupdateAssetsData()completes will throw. Either:
- Declare them as optional and narrow with
!where already guaranteed, or- Initialise with dummy parsers.
133-138: 👍 Concurrent atlas generation.The
Promise.all+console.timepair will shorten the critical path.
142-148: Safeguard against partially-initialised resources.If either parser failed or was aborted, the
itemsRendererconstructor will still be invoked, leading to an exception. Add an early return or guard.- if (resources.version && resources.blockstatesModels && this.itemsAtlasParser && this.blocksAtlasParser) { + if (!resources.blockstatesModels || !this.itemsAtlasParser || !this.blocksAtlasParser) return + + if (resources.version) { resources.itemsRenderer = new ItemsRenderer( resources.version, resources.blockstatesModels, this.itemsAtlasParser, this.blocksAtlasParser ) }
237-237: Null-assertion is safe post-generation.Using
!onatlasParserafterupdateAssetsDatais acceptable given the guarded execution path.
src/resourcesManager.ts
Outdated
| prepareForTransfer () { | ||
| delete this.itemsRenderer | ||
| delete this.worldBlockProvider | ||
| this.customTextures = {} | ||
| return this | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop using delete – assign undefined instead.
delete hurts hidden-class stability and was flagged by Biome. Replacing with plain assignment avoids perf hits and keeps TS typings intact.
- delete this.itemsRenderer
- delete this.worldBlockProvider
- this.customTextures = {}
+ this.itemsRenderer = undefined
+ this.worldBlockProvider = undefined
+ this.customTextures = {}📝 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.
| prepareForTransfer () { | |
| delete this.itemsRenderer | |
| delete this.worldBlockProvider | |
| this.customTextures = {} | |
| return this | |
| } | |
| prepareForTransfer () { | |
| this.itemsRenderer = undefined | |
| this.worldBlockProvider = undefined | |
| this.customTextures = {} | |
| return this | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 52-52: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🤖 Prompt for AI Agents
In src/resourcesManager.ts between lines 50 and 55, replace the use of `delete`
on `this.itemsRenderer` and `this.worldBlockProvider` with assignments to
`undefined`. This change avoids performance issues related to hidden-class
instability and maintains TypeScript typings. Simply assign `this.itemsRenderer
= undefined` and `this.worldBlockProvider = undefined` instead of deleting them.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/resourcesManager.ts (1)
24-45: Mark non-immediately-initialised fields with!(or make them optional).
strictPropertyInitializationwill complain about the newly-added properties that have no initializer.
Recommended patch:- itemsAtlasImage: ImageBitmap - blocksAtlasImage: ImageBitmap - blocksAtlasJson: ItemsAtlasesOutputJson + itemsAtlasImage!: ImageBitmap + blocksAtlasImage!: ImageBitmap + blocksAtlasJson!: ItemsAtlasesOutputJson ... - worldBlockProvider?: WorldBlockProvider + worldBlockProvider?: WorldBlockProvider(The same applies to
itemsRenderer, but that line is unchanged in this diff.)
🧹 Nitpick comments (4)
src/controls.ts (1)
664-666: Simplify the null-safety guard with optional chainingThe two-step check plus
typeofguard can be condensed without losing safety:-if (appViewer.backend?.backendMethods && typeof appViewer.backend.backendMethods.reloadWorld === 'function') { - appViewer.backend.backendMethods.reloadWorld() -} +// `void` keeps the caller fire-and-forget, mirroring `reloadChunks` above +void appViewer.backend?.backendMethods?.reloadWorld?.()• Keeps the intent readable and concise.
•voidmakes it explicit that any returned promise is intentionally ignored; drop it if the result should be awaited/handled.src/resourcesManager.ts (3)
134-140: Guard clause is solid, but consider early-return for clarity.
Minor readability nit – not critical.
192-195: Unnecessary second parser instantiation?
blocksAssetsParseralready hasatlas.latest. Re-creating a newAtlasParserdoubles memory & GC pressure.- this.blocksAtlasParser = new AtlasParser({ latest: blocksAtlas }, blocksCanvas.toDataURL()) + this.blocksAtlasParser = blocksAssetsParser(Apply same idea in
recreateItemsAtlas.)
229-229: Potential null-assertion foot-gun indownloadDebugAtlas.If a caller invokes this before atlases are generated the
!will mask a runtimeundefinedaccess.
Either:
- Guard with
if (!this.itemsAtlasParser) throw …, or- Move the
!into a local assert helper that keeps stack traces clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
renderer/viewer/three/entities.ts(20 hunks)renderer/viewer/three/graphicsBackend.ts(3 hunks)renderer/viewer/three/renderSlot.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(11 hunks)src/controls.ts(1 hunks)src/react/Singleplayer.tsx(1 hunks)src/resourcesManager.ts(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/react/Singleplayer.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- renderer/viewer/three/graphicsBackend.ts
- renderer/viewer/three/renderSlot.ts
- renderer/viewer/three/worldrendererThree.ts
- renderer/viewer/three/entities.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/controls.ts (1)
src/appViewer.ts (1)
appViewer(276-276)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (11)
src/resourcesManager.ts (11)
11-11: No issues with the added import.
The newItemsAtlasesOutputJsontype tightens the contract forblocksAtlasJson.
20-20: Event name addition makes API clearer.
assetsInventoryStartedfills the gap between textures updated and inventory ready.
63-69: Interfaces read cleanly.
Typing the emitter variants separately (TransferredvsCommon) keeps consumer code tidy.
72-72: Nice touch – explicitrestorerName.
Makes the class easy to register with a generic state restorer.
96-96: Good call resetting resources via fresh instance.
Avoids accidental state bleed across RP / version switches.
106-109: 👍 Using null-coalescing keeps hot-reload UX smooth.
No actionable issues.
125-130: Concurrent atlas regeneration is the right move.
Promise.allhalves the wait time in most cases.
146-147:allReadyflag set at the correct point.
Prevents premature consumer access.
155-155: Event emitted before heavy GUI work – nice telemetry hook.
165-165: Method signature stays consistent with sibling helpers.
203-220: Symmetry gap – noitemsAtlasJsonstored.
blocksAtlasJsonis exposed but the items equivalent is lost.
If external callers need item-tile metadata, consider:+ resources.itemsAtlasJson = this.itemsAtlasParser.atlas.latestAdd a typed field to
LoadedResourcesTransferrablefor parity.
| currentResources: LoadedResourcesTransferrable | undefined | ||
| itemsAtlasParser: AtlasParser | ||
| blocksAtlasParser: AtlasParser |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same ! / ? concern for parsers.
Without an initializer these two lines also violate strictPropertyInitialization.
- itemsAtlasParser: AtlasParser
- blocksAtlasParser: AtlasParser
+ itemsAtlasParser!: AtlasParser
+ blocksAtlasParser!: AtlasParser📝 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.
| currentResources: LoadedResourcesTransferrable | undefined | |
| itemsAtlasParser: AtlasParser | |
| blocksAtlasParser: AtlasParser | |
| currentResources: LoadedResourcesTransferrable | undefined | |
| itemsAtlasParser!: AtlasParser | |
| blocksAtlasParser!: AtlasParser |
🤖 Prompt for AI Agents
In src/resourcesManager.ts around lines 81 to 83, the properties
itemsAtlasParser and blocksAtlasParser are declared without initializers,
causing strictPropertyInitialization errors. To fix this, either provide default
initial values for these properties or mark them as possibly undefined using the
? operator. Alternatively, use definite assignment assertion (!) if you are
certain they will be initialized before use. Choose the approach that best fits
the class design to satisfy TypeScript's strict property initialization rules.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
renderer/viewer/lib/basePlayerState.ts (3)
22-22: Choose a realistic defaulteyeHeight
eyeHeightis initialised to0, which renders the camera at the block origin and will very likely break first-person rendering until the first real value arrives.
1.62(survival) or1.54(sneaking) are saner defaults.- eyeHeight: 0, + eyeHeight: 1.62, // default survival eye height
13-13: Remove inline maintenance note from library code
// edit src/mineflayer/playerState.ts ...looks like an IDE reminder.
Leaving TODO/DEV notes in shipped source decreases signal-to-noise and may leak internal paths.Simply drop the line or convert it into a proper TODO with owner/context.
1-4: Import only what you need
ItemSelectoris referenced solely for its type.
Using a type-only import avoids bundling the entire module in environments that don’t strip types.-import { ItemSelector } from 'mc-assets/dist/itemDefinitions' +import type { ItemSelector } from 'mc-assets/dist/itemDefinitions'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.cursor/rules/vars-usage.mdc(1 hunks)renderer/viewer/lib/basePlayerState.ts(1 hunks)src/appViewer.ts(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/vars-usage.mdc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/appViewer.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
| // edit src/mineflayer/playerState.ts for implementation of player state from mineflayer | ||
| export const getInitialPlayerState = () => proxy({ | ||
| playerSkin: undefined as string | undefined, | ||
| inWater: false, | ||
| waterBreathing: false, | ||
| backgroundColor: [0, 0, 0] as [number, number, number], | ||
| ambientLight: 0, | ||
| directionalLight: 0, | ||
| eyeHeight: 0, | ||
| gameMode: undefined as GameMode | undefined, | ||
| lookingAtBlock: undefined as { | ||
| x: number | ||
| y: number | ||
| z: number | ||
| face?: number | ||
| shapes: BlocksShapes | ||
| } | undefined, | ||
| diggingBlock: undefined as { | ||
| x: number | ||
| y: number | ||
| z: number | ||
| stage: number | ||
| face?: number | ||
| mergedShape: BlockShape | undefined | ||
| } | undefined, | ||
| movementState: 'NOT_MOVING' as MovementState, | ||
| onGround: true, | ||
| sneaking: false, | ||
| flying: false, | ||
| sprinting: false, | ||
| itemUsageTicks: 0, | ||
| username: '', | ||
| onlineMode: false, | ||
| lightingDisabled: false, | ||
| shouldHideHand: false, | ||
| heldItemMain: undefined as HandItemBlock | undefined, | ||
| heldItemOff: undefined as HandItemBlock | undefined, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate duplicated sources of truth for movement flags
movementState already captures NOT_MOVING / WALKING / SPRINTING / SNEAKING.
Keeping additional independent booleans (sneaking, flying, sprinting) invites divergence and makes state-sync logic harder to reason about.
Consider collapsing to a single representation or deriving the booleans from movementState via valtio computed getters.
- movementState: 'NOT_MOVING' as MovementState,
- sneaking: false,
- flying: false,
- sprinting: false,
+ movementState: 'NOT_MOVING' as MovementState,
+ /* Derived helpers can be added when really needed:
+ get sneaking() { return this.movementState === 'SNEAKING' },
+ get sprinting() { return this.movementState === 'SPRINTING' },
+ */🤖 Prompt for AI Agents
In renderer/viewer/lib/basePlayerState.ts between lines 13 and 50, the player
state currently uses both a movementState enum and separate boolean flags for
sneaking, flying, and sprinting, which duplicates state and risks
inconsistencies. Refactor by removing the independent boolean flags and instead
derive these states from movementState using computed getters or equivalent
reactive mechanisms in valtio, ensuring a single source of truth for
movement-related state.
| export const getItemSelector = (playerState: PlayerStateRenderer, specificProperties: ItemSpecificContextProperties, item?: import('prismarine-item').Item) => { | ||
| return { | ||
| ...specificProperties, | ||
| 'minecraft:date': new Date(), | ||
| // "minecraft:context_dimension": bot.entityp, | ||
| // 'minecraft:time': bot.time.timeOfDay / 24_000, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused parameter and questionable return shape in getItemSelector
item?: import('prismarine-item').Itemis never referenced → dead code.- Returning a raw
Dateobject forminecraft:datecan break downstream consumers expecting a numeric epoch or string. - The function doesn’t actually use
playerState; consider removing the parameter to avoid caller confusion.
-export const getItemSelector = (playerState: PlayerStateRenderer, specificProperties: ItemSpecificContextProperties, item?: import('prismarine-item').Item) => {
- return {
- ...specificProperties,
- 'minecraft:date': new Date(),
- }
-}
+export const getItemSelector = (
+ specific: ItemSpecificContextProperties & { 'minecraft:date'?: number }
+) => ({
+ ...specific,
+ // milliseconds since epoch keeps the type serialisable
+ 'minecraft:date': specific['minecraft:date'] ?? Date.now(),
+})🤖 Prompt for AI Agents
In renderer/viewer/lib/basePlayerState.ts around lines 62 to 68, remove the
unused 'item' parameter from the getItemSelector function signature, as it is
never referenced. Also, remove the unused 'playerState' parameter since it is
not used inside the function. Change the 'minecraft:date' property to return a
numeric epoch timestamp (e.g., Date.now()) or an ISO string instead of a raw
Date object to ensure compatibility with downstream consumers expecting a
serializable value.
use less dom things for better portability / testing
pick almost all changes
update notification base
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Documentation