[showcase] kradle-ai fork example #419
Conversation
…to_follow auto follow players
…am_changes Merge upstream changes from zardoy with current next branch
…ments Allan/recording playback improvements
…skins KRA-386 Custom Agent Skin Loading
|
@allan-sqf is attempting to deploy a commit to the Vitaly Turovksy's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughAdds a new AppViewer orchestration and Three.js graphics backend (WorldRendererThree), large refactors across renderer/mesher/entities/resources, build/workflow additions (single-file, zip, CI), replay and iframe integrations, new utilities (progress reporters, timers, GUI atlas), many API/type surface changes, and removal of legacy Viewer files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI
participant App as AppViewer
participant GB as GraphicsBackend
participant DR as DocumentRenderer
participant WR as WorldRendererThree
participant WV as WorldDataEmitter
UI->>App: loadBackend(threejs loader)
App->>GB: createGraphicsBackend(initOptions)
GB->>DR: new DocumentRenderer()
App->>WV: new WorldDataEmitter()
App->>GB: startWorld({ version, worldView: WV, ... })
GB->>WR: new WorldRendererThree(renderer, initOptions, displayOptions)
WR->>WV: connect(events)
App-->>UI: resolve worldReady
loop render frames
DR->>WR: render(sizeChanged?)
WR-->>DR: frame done
end
sequenceDiagram
autonumber
participant Parent as Kradle Frontend
participant Iframe as Web Client
Parent-->>Iframe: postMessage({ source: 'kradle-frontend', action, ... })
Iframe->>Iframe: map action → emit customEvents('kradle:<action>')
Iframe-->>Parent: postMessage({ source: 'minecraft-web-client', action: 'connectionStatus', data })
Iframe-->>Parent: postMessage({ source: 'minecraft-web-client', action: 'replayStatus', data })
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@allan-sqf Good work! Can you tell a little more of your case? I can see only work on features & code removal, I wonder what was your case? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 106
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (36)
src/markdownToFormattedText.ts (2)
12-37: Normalize transform output, preserve nested formatting, and avoid dropping links/unknown nodes.Current returns are inconsistent (string | object | array | undefined), bold/italic only uses first child, and unknown nodes (e.g., links) get dropped because value is often undefined. Normalize to MCComponent[] everywhere, flatten children, and apply styles to all descendants.
Apply this diff:
-function transformToMinecraftJSON (element: Element): any { - switch (element.type) { - case 'root': { - if (!element.children) return - return element.children.map(child => transformToMinecraftJSON(child)).filter(Boolean) - } - case 'paragraph': { - if (!element.children) return - const transformedChildren = element.children.map(child => transformToMinecraftJSON(child)).filter(Boolean) - return transformedChildren.flat() - } - case 'strong': { - if (!element.children) return - return [{ bold: true, text: element.children[0].value }] - } - case 'text': { - return { text: element.value } - } - case 'emphasis': { - if (!element.children) return - return [{ italic: true, text: element.children[0].value }] - } - default: - // todo leave untouched eg links - return element.value - } -} +function transformToMinecraftJSON (element: Element): MCComponent[] { + const flatten = (els?: Element[]) => (els ?? []).flatMap(transformToMinecraftJSON) + switch (element.type) { + case 'root': + case 'paragraph': + return flatten(element.children) + case 'text': + return element.value ? [{ text: element.value }] : [] + case 'strong': + return flatten(element.children).map(c => ({ ...c, bold: true })) + case 'emphasis': + return flatten(element.children).map(c => ({ ...c, italic: true })) + default: + // Fallback: keep unknown nodes' textual content (e.g., links) and their children. + return element.children ? flatten(element.children) : (element.value ? [{ text: element.value }] : []) + } +}
24-26: Bold/italic currently drop all but the first child.Using element.children[0] loses content and nesting (e.g., mixed text in a single ). The refactor above applies styles to all descendant components.
Also applies to: 30-33
renderer/viewer/lib/animationController.ts (2)
72-76: Cancellation can hang:cancelCurrentAnimation()never triggers completion without an external call.If
cancelCurrentAnimation()is awaited (e.g., via callers that don’t pre-forceFinish), its promise may never resolve because nothing invokesanimationCycleFinish(). Call it fromupdate()to flush cancellations deterministically.Apply this diff:
update () { - if (this.currentAnimation) { - this.currentAnimation.update() - } + if (this.currentAnimation) { + this.currentAnimation.update() + this.animationCycleFinish() + } }
61-69: ClearcurrentCancelCallbackon finish to avoid stale references.Without clearing it on any finish path, you retain a closure longer than needed.
Apply this diff:
this.isAnimating = false this.cancelRequested = false + this.currentCancelCallback = null const callbacks = [...this.completionCallbacks] this.completionCallbacks = [] if (callComplete) { for (const cb of callbacks) cb() }renderer/viewer/lib/smoothSwitcher.ts (2)
38-40:defaultStateis captured by reference;reset()won’t restore defaults.Clone the initial state so mutations don’t change the baseline.
Apply this diff:
- this.defaultState = this.getState() + this.defaultState = { ...this.getState() }
49-56: Handle undefined properties in duration calc; fix comment mismatch.Undefined current values produce
NaNduration; also comment says 50ms but code clamps to 200ms.Apply this diff:
- for (const [key, targetValue] of Object.entries(newState)) { - const currentValue = currentState[key] - const diff = Math.abs(targetValue! - currentValue) + for (const [key, targetValue] of Object.entries(newState)) { + const currentValue = currentState[key] ?? 0 + const diff = Math.abs(targetValue! - currentValue) const speed = this.getPropertySpeed(key) const duration = (diff / speed) * 1000 // Convert to milliseconds maxDuration = Math.max(maxDuration, duration) } - // Ensure minimum duration of 50ms and maximum of 2000ms + // Ensure minimum duration of 200ms and maximum of 2000ms return Math.min(Math.max(maxDuration, 200), 2000)Also applies to: 58-60
scripts/prepareSounds.mjs (4)
146-153: Replace exec + ts-ignore with spawn and stream stderr (ffmpeg logs to stderr).This removes the type suppression and avoids exec’s maxBuffer pitfalls.
- const proc = promisify(exec)(`${ffmpegExec} -i "${toConvert[i]}" -y -codec:a libmp3lame ${maintainBitrate ? '-qscale:a 2' : ''} "${toConvert[i].replace('.ogg', '.mp3')}"`) - // pipe stdout to the console - //@ts-ignore - proc.child.stdout.pipe(process.stdout) - await proc + const args = ['-i', toConvert[i], '-y', '-codec:a', 'libmp3lame'] + if (maintainBitrate) args.push('-qscale:a', '2') + args.push(toConvert[i].replace('.ogg', '.mp3')) + const child = spawn(ffmpegExec, args, { stdio: ['ignore', 'inherit', 'inherit'] }) + await new Promise((resolve, reject) => { + child.on('error', reject) + child.on('close', (code) => (code === 0 ? resolve() : reject(new Error(`ffmpeg exited with code ${code}`)))) + })Additional import change required:
// at top of file - import { exec } from 'child_process' - import { promisify } from 'util' + import { spawn } from 'child_process'
249-254: raw.githubusercontent.com URL is missing the branch segment. Parameterize branch and base dir.As written, the URL will 404 unless REPO_SLUG includes a branch (unusual). Fix by adding REPO_BRANCH (defaulting to “sounds”) and SOUNDS_BASE_DIR.
- if (!process.env.REPO_SLUG) throw new Error('REPO_SLUG is not set') + const REPO_SLUG = process.env.REPO_SLUG + if (!REPO_SLUG) throw new Error('REPO_SLUG is not set') + const REPO_BRANCH = process.env.REPO_BRANCH || 'sounds' + const SOUNDS_BASE_DIR = process.env.SOUNDS_BASE_DIR || 'sounds' @@ - const allSoundsMeta = { - format: 'mp3', - baseUrl: `https://raw.githubusercontent.com/${process.env.REPO_SLUG}/sounds/sounds/` - } + const allSoundsMeta = { + format: 'mp3', + baseUrl: `https://raw.githubusercontent.com/${REPO_SLUG}/${REPO_BRANCH}/${SOUNDS_BASE_DIR}/` + }
28-31: Initialize and normalize existingSoundsCache.sounds; current shape is inconsistent and can throw.You read nested keys via reduce but write flat keys (
sounds[namePath] = true). Alsosoundsmay be undefined, causing a crash on write.let existingSoundsCache = {} try { existingSoundsCache = JSON.parse(await fs.promises.readFile(EXISTING_CACHE_PATH, 'utf8')) } catch (err) {} + // ensure shape + existingSoundsCache.sounds ??= {}- const cached = - !!namePath.replace('.ogg', '.mp3').split('/').reduce((acc, cur) => acc?.[cur], existingSoundsCache.sounds) || - !!namePath.replace('.ogg', '.ogg').split('/').reduce((acc, cur) => acc?.[cur], existingSoundsCache.sounds) + const cached = + !!existingSoundsCache.sounds[namePath.replace('.ogg', '.mp3')] || + !!existingSoundsCache.sounds[namePath.replace('.ogg', '.ogg')] @@ - existingSoundsCache.sounds[namePath] = true + existingSoundsCache.sounds[namePath.replace('.ogg', '.mp3')] = trueAlso applies to: 71-79
167-170: Guard the initial Burger fetch used for burger.json.This call can still fail and abort the whole script (now that the default path runs). Mirror the per-version try/catch and status checks.
- const burgerData = await fetch(burgerDataUrl(targetedVersions[0])).then((r) => r.json()) - fs.writeFileSync(burgerDataPath, JSON.stringify(burgerData[0].sounds), 'utf8') + try { + const resp = await fetch(burgerDataUrl(targetedVersions[0])) + if (!resp.ok) throw new Error(`HTTP ${resp.status}`) + const burgerData = await resp.json() + fs.writeFileSync(burgerDataPath, JSON.stringify(burgerData[0].sounds), 'utf8') + } catch (err) { + console.error('error fetching initial burger data', targetedVersions[0], err) + // decide whether to continue or abort; current code assumes presence -> safest to return + return + }scripts/makeOptimizedMcData.mjs (2)
9-16: Fix ESM default import interop: JsonOptimizer becomes undefined at runtime.
import { default as _JsonOptimizer } ...already gives you the default export. Accessing._defaultmakesJsonOptimizerundefined and breaksnew JsonOptimizer(...).Apply this diff:
-/** @type {typeof _JsonOptimizer} */ -//@ts-ignore -const JsonOptimizer = _JsonOptimizer.default +/** @type {typeof _JsonOptimizer} */ +const JsonOptimizer = _JsonOptimizer
258-270: Always run preprocessors (processData), even for raw types.This enables
processRecipeswithout forcing diffs for recipes.Apply this diff:
- const dataRaw = getData(dataType) - let rawData = dataRaw - if (config.raw) { + const dataRaw = getData(dataType) + // Allow a preprocessing pass regardless of raw/diff mode + try { + config.processData?.(dataRaw, previousData[dataType], getData, version) + } catch (err) { + const error = new Error(`Failed to preprocess ${dataType} for ${version}: ${err.message}`) + error.stack = err.stack + throw error + } + let rawData = dataRaw + if (config.raw) { rawDataVersions[dataType] ??= {} rawDataVersions[dataType][version] = rawData rawData = dataRaw } else { - if (!diffSources[dataType]) { + if (!diffSources[dataType]) { diffSources[dataType] = new JsonOptimizer(config.arrKey, config.ignoreChanges, config.ignoreRemoved) } - try { - config.processData?.(dataRaw, previousData[dataType], getData, version) - diffSources[dataType].recordDiff(version, dataRaw) + try { + diffSources[dataType].recordDiff(version, dataRaw) injectCode = `restoreDiff(sources, ${JSON.stringify(dataType)}, ${JSON.stringify(version)})` } catch (err) { const error = new Error(`Failed to diff ${dataType} for ${version}: ${err.message}`) error.stack = err.stack throw error } }renderer/playground/scenes/frequentUpdates.ts (1)
62-70: Fix tick-rate math: wrong parentheses cause overscheduling and time drift
Math.floor(performance.now() - lastTime) / updateEachfloors milliseconds before division, yielding fractional update counts and extra updates. Also, resettinglastTimetoperformance.now()discards remainder time and drifts.Apply:
- const updateEach = 1000 / this.params.testUpdatesPerSecond + const rate = this.params.testUpdatesPerSecond + if (rate <= 0 || !Number.isFinite(rate)) return + const updateEach = 1000 / rate requestAnimationFrame(tick) if (!this.params.testActive) return - const updateCount = Math.floor(performance.now() - lastTime) / updateEach + const now = performance.now() + const updateCount = Math.floor((now - lastTime) / updateEach) for (let i = 0; i < updateCount; i++) { this.updateBlock() } - lastTime = performance.now() + if (updateCount > 0) lastTime += updateCount * updateEachsrc/chatUtils.ts (1)
47-53: Avoid mutating input messages; derive a local text value.Mutating
msg.textcan leak side effects to callers. Use a localtextinstead.Apply:
- if (!msg.text && typeof msg.json?.[''] === 'string') msg.text = msg.json[''] - if (msg.text) { - msglist.push({ - ...msg, - text: msg.text, - ...styles - }) - } else if (msg.translate) { + const text = msg.text ?? (typeof msg.json?.[''] === 'string' ? msg.json[''] : undefined) + if (text) { + msglist.push({ + ...msg, + text, + ...styles + }) + } else if (msg.translate) {src/basicSounds.ts (1)
46-55: Race-safe timeout via Promise.race and clearer variable naming.Don’t rely on wall-clock measurement; race the loader against a real timer and use a semantically named flag.
-export const loadOrPlaySound = async (url, soundVolume = 1, loadTimeout = 500) => { - const soundBuffer = sounds[url] - if (!soundBuffer) { - const start = Date.now() - const cancelled = await loadSound(url) - if (cancelled || Date.now() - start > loadTimeout) return - } +export const loadOrPlaySound = async (url: string, soundVolume = 1, loadTimeout = 500) => { + if (!sounds[url]) { + const timeoutP = new Promise<boolean>(resolve => setTimeout(() => resolve(false), loadTimeout)) + const loaded = await Promise.race([loadSound(url), timeoutP]) + if (!loaded) return + } return playSound(url, soundVolume) }README.MD (1)
103-110: Fix typo in “browser” and minor grammar.Public docs—quick polish.
-**Opera Mini**: Disable *mouse gestures* in browsre settings to avoid opening new tab on right click hold +**Opera Mini**: Disable mouse gestures in browser settings to avoid opening a new tab on right‑click hold.renderer/viewer/three/world/vr.ts (2)
141-144: Wrong hand referenced on controller2 disconnect.Uses hand1 instead of hand2, leading to inconsistent virtual gamepad teardown.
- controller2.addEventListener('disconnected', () => { - manageXrInputSource(hand1.xrInputSource, 'right', true) - hand2.xrInputSource = undefined - }) + controller2.addEventListener('disconnected', () => { + manageXrInputSource(hand2.xrInputSource, 'right', true) + hand2.xrInputSource = undefined + })
149-163: Preserve fixed-length Gamepad array semantics
Overridenavigator.getGamepads()by cloning the original array and assigning your virtual controller atvirtualGamepadIndexinstead of appending, so consumers relying on a fixed-length array (length = max index + 1, with nulls for empty slots) won’t break.- //@ts-expect-error - navigator.getGamepads = () => { - const originalGamepads = originalGetGamepads() - if (!hand1.xrInputSource || !hand2.xrInputSource) return originalGamepads - return [ - ...originalGamepads, - { - axes: remapAxes(hand2.xrInputSource.gamepad.axes, hand1.xrInputSource.gamepad.axes), - buttons: remapButtons(hand2.xrInputSource.gamepad.buttons, hand1.xrInputSource.gamepad.buttons), - connected: true, - mapping: 'standard', - id: '', - index: virtualGamepadIndex - } - ] - } + //@ts-expect-error + navigator.getGamepads = () => { + const pads = originalGetGamepads() + if (!hand1.xrInputSource || !hand2.xrInputSource) return pads + const clone = Array.from(pads) + const virtual = { + axes: remapAxes(hand2.xrInputSource.gamepad.axes, hand1.xrInputSource.gamepad.axes), + buttons: remapButtons(hand2.xrInputSource.gamepad.buttons, hand1.xrInputSource.gamepad.buttons), + connected: true, + mapping: 'standard', + id: 'XR-virtual', + index: virtualGamepadIndex + } + clone[virtualGamepadIndex] = virtual as any + return clone as Gamepad[] + }Validate in-app consumers to catch any use of fixed-length assumptions.
src/inventoryWindows.ts (2)
139-157: Guard resource access in getImageSrc to avoid undefined currentResources.Ensure resources are loaded before dereferencing atlas parsers.
-const getImageSrc = (path): string | HTMLImageElement => { +const getImageSrc = (path): string | HTMLImageElement => { + const resources = appViewer.resourcesManager.currentResources + assertDefined(resources) switch (path) { case 'gui/container/inventory': return appReplacableResources.latest_gui_container_inventory.content - case 'blocks': return appViewer.resourcesManager.currentResources!.blocksAtlasParser.latestImage - case 'items': return appViewer.resourcesManager.currentResources!.itemsAtlasParser.latestImage + case 'blocks': return resources.blocksAtlasParser.latestImage + case 'items': return resources.itemsAtlasParser.latestImage
551-554: Use numeric comparator when sorting recipe ingredient IDs.Default sort is lexicographic; breaks shapeless recipe detection.
- const slotsIngredients = [...inputSlotsItems].sort().filter(item => item !== undefined) + const slotsIngredients = [...inputSlotsItems] + .sort((a, b) => ((a ?? 0) - (b ?? 0))) + .filter(item => item !== undefined)renderer/viewer/three/entity/EntityMesh.ts (1)
484-486: Offset fallback should mirror scale logic.For mapped types (e.g., spectral_arrow → arrow), use type fallback when originalType lacks an offset.
- const offset = offsetEntity[originalType] + const offset = offsetEntity[originalType] || offsetEntity[type]renderer/viewer/lib/worldrendererCommon.ts (1)
758-770: Race after destroy: pending dispatchMessages timer can post to terminated workersIf destroy() is called before the timeout fires, worker will be undefined and postMessage will throw. Guard for active state and missing worker.
Apply this diff:
dispatchMessages () { if (this.queueAwaited) return this.queueAwaited = true setTimeout(() => { + if (!this.active) { // world was destroyed + this.messagesQueue = {} + this.queueAwaited = false + return + } // group messages and send as one for (const workerIndex in this.messagesQueue) { - const worker = this.workers[Number(workerIndex)] - worker.postMessage(this.messagesQueue[workerIndex]) + const worker = this.workers[Number(workerIndex)] + if (!worker) continue + worker.postMessage(this.messagesQueue[workerIndex]) } this.messagesQueue = {} this.queueAwaited = false }) }renderer/viewer/lib/mesher/test/tests.test.ts (1)
50-58: Snapshot will be brittle across MC versions; prefer semantic assertionInline snapshot with creaking_heart ties the test to a specific data state and will churn with version bumps. Assert a minimal set and allow extras.
Apply this diff:
- expect(missingBlocks).toMatchInlineSnapshot(` - { - "creaking_heart": true, - "end_gateway": true, - "end_portal": true, - "structure_void": true, - } - `) + expect(missingBlocks).toMatchObject( + expect.objectContaining({ + end_gateway: true, + end_portal: true, + structure_void: true, + }) + )renderer/playground/scenes/main.ts (3)
115-118: Bug: wrong path to player username (entities.idvsentities['id']).
viewer.entities.entities.idis almost certainly undefined; you use bracket form elsewhere. Fix to avoid runtime errors.- viewer.entities.updatePlayerSkin('id', viewer.entities.entities.id.username, undefined, true, true) + viewer.entities.updatePlayerSkin('id', viewer.entities.entities['id'].username, undefined, true, true)
162-200: Restore renderer/camera state after bundle capture.
blockIsomorphicRenderBundlemutates canvas size, camera, background, light, andignoreResizewithout restoring them, leaving the app in an altered state after export.blockIsomorphicRenderBundle () { const { renderer } = viewer const canvas = renderer.domElement + // snapshot current state to restore later + const prev = { + ignoreResize: this.ignoreResize, + canvasWidth: canvas.width, + canvasHeight: canvas.height, + rendererSize: renderer.getSize(new THREE.Vector2()), + camera: viewer.camera, + background: viewer.scene.background, + lightPos: viewer.directionalLight.position.clone(), + lightIntensity: viewer.directionalLight.intensity, + } @@ renderer.setSize(size, size) @@ viewer.scene.background = null @@ const end = async () => { // download zip file @@ viewer.world.renderUpdateEmitter.removeListener('update', onWorldUpdate) + // restore previous state + this.ignoreResize = prev.ignoreResize + renderer.setSize(prev.rendererSize.x, prev.rendererSize.y) + canvas.width = prev.canvasWidth + canvas.height = prev.canvasHeight + viewer.camera = prev.camera + viewer.scene.background = prev.background + viewer.directionalLight.position.copy(prev.lightPos) + viewer.directionalLight.intensity = prev.lightIntensity }Also applies to: 227-241
201-209:updateBlock()doesn’t apply changes. Invoke handlers to actually update world/render.Right now it only mutates
params.block. Call your existing update hooks.const updateBlock = () => { // viewer.setBlockStateId(targetPos, mcData.blocksByName[blockName].minStateId) this.params.block = blockName - // todo cleanup (introduce getDefaultState) - // TODO - // onUpdate.block() - // applyChanges(false, true) + // apply param-level updates and propagate to world + this.onParamUpdate.block() + this.onParamsUpdate('block', {}) }src/connect.ts (1)
46-63: Guard_LOAD_MC_DATAand validate version inloadMinecraftData.
Add optional chaining to avoid calling an undefined preloader and throw on unsupported versions before usingminecraftVersion. Consider defining specificwindowproperties inglobals.d.tsfor stronger typing.-export const loadMinecraftData = async (version: string) => { - await window._LOAD_MC_DATA() +export const loadMinecraftData = async (version: string) => { + await window._LOAD_MC_DATA?.() @@ - const mcData = MinecraftData(version) - window.PrismarineBlock = PrismarineBlock(mcData.version.minecraftVersion!) - window.PrismarineItem = PrismarineItem(mcData.version.minecraftVersion!) + const mcData = MinecraftData(version) + const mcVersion = mcData.version.minecraftVersion + if (!mcVersion) throw new Error(`Unsupported Minecraft version: ${version}`) + window.PrismarineBlock = PrismarineBlock(mcVersion) + window.PrismarineItem = PrismarineItem(mcVersion)src/downloadAndOpenFile.ts (1)
136-136: Type-safe error message in catch.err is unknown in TS. Guard it to avoid TS errors and odd messages.
- setLoadingScreenStatus(`Failed to download. Either refresh page or remove map param from URL. Reason: ${err.message}`) + const msg = err instanceof Error ? err.message : String(err) + setLoadingScreenStatus(`Failed to download. Either refresh page or remove map param from URL. Reason: ${msg}`)src/builtinCommands.ts (1)
115-133: Guard against pre-login usage and missing positions in /pos.If the bot isn’t logged in yet,
bot.entitymay be undefined and this will throw. Also,followingcan be unset. Add guards and fallbacks.Apply:
- async invoke ([type]) { + async invoke ([type]) { let pos: { x: number, y: number, z: number } | undefined if (type === 'block') { const blockPos = window.cursorBlockRel()?.position if (blockPos) { pos = { x: blockPos.x, y: blockPos.y, z: blockPos.z } } } else { - const playerPos = following?.entity?.position ?? bot.entity.position - pos = { x: playerPos.x, y: playerPos.y, z: playerPos.z } + const playerPos = following?.entity?.position ?? bot?.entity?.position + if (playerPos) pos = { x: playerPos.x, y: playerPos.y, z: playerPos.z } } if (!pos) return const formatted = `${pos.x.toFixed(2)} ${pos.y.toFixed(2)} ${pos.z.toFixed(2)}` // await navigator.clipboard.writeText(formatted) writeText(`Position: ${formatted}`) }src/appParams.ts (1)
103-106: Ensure helpers reflect current URL state.
hasQueryParamandgetRawQueryParamsshould not return stale data.-export const hasQueryParam = (param: keyof AppQsParams) => qsParams.has(param) +export const hasQueryParam = (param: keyof AppQsParams) => { + return new URLSearchParams(window.location.search).has(param) +} -export const getRawQueryParams = () => qsParams; +export const getRawQueryParams = () => new URLSearchParams(window.location.search)rsbuild.config.ts (2)
210-214: Awaitprep()in build hooks to avoid race conditions.Not awaiting
prep()can cause builds to start before assets are prepared.Apply this diff:
- build.onBeforeBuild(async () => { - prep() - }) + build.onBeforeBuild(async () => { + await prep() + })
245-246: Also awaitprep()before starting the dev server.Ensure the dev server starts with prepared assets.
Apply this diff:
- build.onBeforeStartDevServer(() => prep()) + build.onBeforeStartDevServer(async () => { + await prep() + })renderer/viewer/lib/mesher/world.ts (1)
235-239: Don’t mutate shared cached blocks with position-dependentbiome.Blocks in
blockCacheare shared across positions. Overwritingblock.biomemakes results position-nondeterministic for other callers.Apply this diff:
- //@ts-expect-error - block.biome = this.biomeCache[column.getBiome(locInChunk)] ?? this.biomeCache[1] ?? this.biomeCache[0] - if (block.name === 'redstone_ore') block.transparent = false - return block + const biome = this.biomeCache[column.getBiome(locInChunk)] ?? this.biomeCache[1] ?? this.biomeCache[0] + // Shadow `biome` on a per-call wrapper to keep cache pure + const blockWithBiome = Object.create(block) + Object.defineProperty(blockWithBiome, 'biome', { value: biome, enumerable: true }) + if (block.name === 'redstone_ore') block.transparent = false + return blockWithBiomerenderer/viewer/three/holdingBlock.ts (2)
371-392: Dispose previous model (and its textures) to prevent GPU leaks.When replacing the held item, the previous object is only detached. Ensure we call any
additionalCleanuphook and dispose geometries/materials.async replaceItemModel (handItem?: HandItemBlock): Promise<void> { @@ - if (!handItem) { - this.holdingBlock?.removeFromParent() + if (!handItem) { + // dispose previous + // @ts-expect-error + this.holdingBlock?.additionalCleanup?.() + disposeObject(this.holdingBlock, true) this.holdingBlock = undefined @@ - // Update the model without changing the group structure - this.holdingBlock?.removeFromParent() + // Update the model without changing the group structure + // @ts-expect-error + this.holdingBlock?.additionalCleanup?.() + disposeObject(this.holdingBlock, true) this.holdingBlock = result.model
403-461: Also dispose on full re-build during switch animation.Mirror the disposal in
setNewItemfor the disappear path.async setNewItem (handItem?: HandItemBlock) { @@ - this.holdingBlock?.removeFromParent() + // @ts-expect-error + this.holdingBlock?.additionalCleanup?.() + disposeObject(this.holdingBlock, true) this.holdingBlock = undefinedrenderer/viewer/three/entities.ts (1)
560-579: Attach per-item cleanup to prevent texture leaks.Item path clones two textures. Attach an
additionalCleanuphook on the returned mesh so holders (e.g., HoldingBlock) can dispose them on swap.const mesh = new THREE.Mesh(new THREE.BoxGeometry(1, 1, 0), [ @@ ]) + // allow holders to dispose these at swap-time + // @ts-expect-error + mesh.additionalCleanup = () => { + itemsTexture?.dispose() + itemsTextureFlipped?.dispose() + }
| - name: Update deployAlwaysUpdate packages | ||
| run: | | ||
| if [ -f package.json ]; then | ||
| PACKAGES=$(node -e "const pkg = require('./package.json'); if (pkg.deployAlwaysUpdate) console.log(pkg.deployAlwaysUpdate.join(' '))") | ||
| if [ ! -z "$PACKAGES" ]; then | ||
| echo "Updating packages: $PACKAGES" | ||
| pnpm up -L $PACKAGES | ||
| else | ||
| echo "No deployAlwaysUpdate packages found in package.json" | ||
| fi | ||
| else | ||
| echo "package.json not found" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden the deployAlwaysUpdate step (quoting, safer test, fail-fast).
Quote PACKAGES, prefer [ -n ... ], add -- before args, and enable strict shell flags to avoid subtle CI breakages.
- - name: Update deployAlwaysUpdate packages
- run: |
- if [ -f package.json ]; then
- PACKAGES=$(node -e "const pkg = require('./package.json'); if (pkg.deployAlwaysUpdate) console.log(pkg.deployAlwaysUpdate.join(' '))")
- if [ ! -z "$PACKAGES" ]; then
- echo "Updating packages: $PACKAGES"
- pnpm up -L $PACKAGES
- else
- echo "No deployAlwaysUpdate packages found in package.json"
- fi
- else
- echo "package.json not found"
- fi
+ - name: Update deployAlwaysUpdate packages
+ run: |
+ set -euo pipefail
+ if [ -f package.json ]; then
+ PACKAGES="$(node -e "const pkg = require('./package.json'); if (pkg.deployAlwaysUpdate) console.log(pkg.deployAlwaysUpdate.join(' '))")"
+ if [ -n "$PACKAGES" ]; then
+ echo "Updating packages: $PACKAGES"
+ pnpm up -L -- $PACKAGES
+ else
+ echo "No deployAlwaysUpdate packages found in package.json"
+ fi
+ else
+ echo "package.json not found"
+ fi📝 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.
| - name: Update deployAlwaysUpdate packages | |
| run: | | |
| if [ -f package.json ]; then | |
| PACKAGES=$(node -e "const pkg = require('./package.json'); if (pkg.deployAlwaysUpdate) console.log(pkg.deployAlwaysUpdate.join(' '))") | |
| if [ ! -z "$PACKAGES" ]; then | |
| echo "Updating packages: $PACKAGES" | |
| pnpm up -L $PACKAGES | |
| else | |
| echo "No deployAlwaysUpdate packages found in package.json" | |
| fi | |
| else | |
| echo "package.json not found" | |
| fi | |
| - name: Update deployAlwaysUpdate packages | |
| run: | | |
| set -euo pipefail | |
| if [ -f package.json ]; then | |
| PACKAGES="$(node -e "const pkg = require('./package.json'); if (pkg.deployAlwaysUpdate) console.log(pkg.deployAlwaysUpdate.join(' '))")" | |
| if [ -n "$PACKAGES" ]; then | |
| echo "Updating packages: $PACKAGES" | |
| pnpm up -L -- $PACKAGES | |
| else | |
| echo "No deployAlwaysUpdate packages found in package.json" | |
| fi | |
| else | |
| echo "package.json not found" | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/preview.yml around lines 55 to 67: the deployAlwaysUpdate step
is fragile — PACKAGES is unquoted, the test uses [ ! -z ... ], arguments to pnpm
can be misinterpreted, and the shell lacks strict flags; update the step to
enable strict mode (set -euo pipefail), quote variable expansions ("$PACKAGES"),
use [ -n "$PACKAGES" ] for the non-empty check, pass `--` before $PACKAGES to
pnpm (pnpm up -L -- "$PACKAGES") so package names aren't treated as options, and
ensure the script fails fast on errors so the CI stops on failure.
| @@ -0,0 +1 @@ | |||
| ** | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repo-wide Prettier disable (**) is high risk; restrict to generated/build artifacts.
Disabling formatting everywhere invites style drift and noisy diffs. Ignore only generated/output paths instead.
Apply:
-**
+# Keep source formatted; ignore only generated/build outputs
+node_modules/
+dist/
+build/
+coverage/
+cypress/screenshots/
+cypress/videos/
+package/
+*.zip
+src/generated/
+generated/If the intent was to prevent formatting of src/generated/resources.ts, the above suffices without turning Prettier off globally.
📝 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.
| ** | |
| # Keep source formatted; ignore only generated/build outputs | |
| node_modules/ | |
| dist/ | |
| build/ | |
| coverage/ | |
| cypress/screenshots/ | |
| cypress/videos/ | |
| package/ | |
| *.zip | |
| src/generated/ | |
| generated/ |
🤖 Prompt for AI Agents
.prettierignore lines 1-1: remove the global '**' entry (which disables Prettier
repo-wide) and instead add only the actual generated/output paths you want
excluded (for example: build/, dist/, node_modules/, .next/, and specific
generated files like src/generated/resources.ts). Update the file to list those
concrete directories/files and do not include a blanket wildcard so formatting
remains enabled for the rest of the repo.
| - If CI is failing on the next branch for some reason, feel free to use the latest commit for release branch. We will update the base branch asap. Please, always make sure to allow maintainers do changes when opening PRs. | ||
|
|
||
| ## Tasks Categories | ||
| ## Tasks Categories (from usptream repo. not relevant to Kradle) |
There was a problem hiding this comment.
Fix typos and project reference in section header.
“usptream” → “upstream”; “Kradle” doesn’t match this repo.
Apply:
-## Tasks Categories (from usptream repo. not relevant to Kradle)
+## Tasks Categories (from upstream repo; not specific to this project)📝 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.
| ## Tasks Categories (from usptream repo. not relevant to Kradle) | |
| ## Tasks Categories (from upstream repo; not specific to this project) |
🤖 Prompt for AI Agents
In CONTRIBUTING.md around line 96, the section header contains typos and an
incorrect project reference; change “Tasks Categories (from usptream repo. not
relevant to Kradle)” to a corrected header such as “Tasks Categories (from
upstream repo — not relevant to this project)” by fixing “usptream” to
“upstream” and replacing “Kradle” with a neutral reference like “this project”
(or the correct repo name if intended).
There was a problem hiding this comment.
What is kradle? How changes in or are prepared for platform?
There was a problem hiding this comment.
🧩 Analysis chain
@zardoy Based on my analysis of the codebase, Kradle appears to be a platform that integrates and hosts this Minecraft Web Client. Here's what I found:
What is Kradle?
Kradle seems to be a service/platform that:
-
Hosts the Minecraft Web Client in iframes - The code includes extensive iframe communication between the minecraft-web-client and "kradle frontend"
-
Provides proxy services - The config shows
defaultProxy: "https://mcproxy.kradle.ai" -
Offers enhanced multiplayer features - Including custom agent skins, follow functionality, and real-time status reporting
How changes are prepared for Kradle platform:
1. Bi-directional Communication (src/iframe.ts)
- Listens for messages from kradle frontend
- Sends status updates back to Kradle:
- Game loaded status
- Connection status
- Replay status from boss bar packets
- Following player updates
2. Custom Kradle Events
The platform supports these custom commands:
kradle:command- Execute bot commandskradle:followPlayer- Follow specific playerskradle:reconnect- Reconnect functionalitykradle:setAgentSkins- Custom agent skin managementkradle:setVolume/kradle:setMusic- Audio controls
3. Platform-specific Features
- Agent skin mapping (
window.agentSkinMap) for custom player skins - Connection status reporting with detailed error handling
- Replay monitoring that parses boss bar data for replay progress
- Follow system integrated with Kradle's user management
4. Deployment Integration
- Uses environment variables for configuration
- Supports both bundled and remote config loading
- Custom build processes for Kradle-specific deployments
The extensive integration suggests Kradle is likely a multiplayer Minecraft platform that uses this web client as its game engine, similar to how platforms like Hypixel or Mineplex might integrate game clients, but built specifically for web-based Minecraft experiences.
🌐 Web query:
Kradle platform Minecraft web client
💡 Result:
Kradle is an AI platform that uses Minecraft as its environment: challenges are hosted on Kradle and runs spawn instances in Minecraft where participants (agents) are controlled by your code. Agents receive JSON observations (game events) and return actions (JavaScript code, often using Mineflayer skills). Kradle provides docs, a Python SDK helper, a skills/examples repo, and tools for creating/chaining challenges and agent modes. Key points:
- Environment: Minecraft-based challenges (hosted by Kradle). [1]
- Control loop: Kradle POSTs observations to your agent’s web endpoints (/init and /event); your agent responds with actions (JS code). [2]
- Tooling: SDKs, a skills library (Mineflayer-based) and example commands on GitHub; advanced features and agent modes documented. [3]
- Legal/usage: platform terms and restrictions (accounts, acceptable use). [4]
If you want, I can:
- show the API observation/action JSON examples,
- link the GitHub examples and SDK,
- or walk through a minimal agent that responds to an observation.
Sources: [1] Kradle “What’s Kradle?” docs, [2] Kradle “How Kradle works” docs, [3] Kradle “Advanced features” (skills/examples), [4] Kradle terms of use.
🌐 Web query:
kradle.ai Minecraft platform
💡 Result:
Kradle.ai is a platform that runs open-ended, interactive AI “evals” inside Minecraft so developers can build and test agents in multiplayer, dynamic challenges. Key points:
- Purpose: crowd-sourced, interactive, multi‑agent evaluations (not static datasets) to benchmark and stress‑test adaptive agent behavior. [1]
- Environment: uses Minecraft worlds (survival/creative, PvP/PvE, multi‑role, custom rules) so tasks can range from simple resource collection to complex multi‑agent coordination. [1]
- API/agent model: agents run as web services (init and event POST endpoints). Kradle sends JSON observations (state, chat, events) and accepts actions as JavaScript (Mineflayer-based) or via SDKs (Python helper). [2]
- Features: skills library (prebuilt actions), agent modes (self_preservation, unstuck, etc.), challenge creation and hosted runs. Example commands and examples are on their GitHub. [3]
Sources: Kradle docs and site. [1][2][3]
@zardoy Thank you for the clarification! I now have a much clearer understanding of what Kradle actually is. Let me correct my previous response:
What is Kradle?
Kradle is an AI platform that uses Minecraft as its environment for running interactive AI agent evaluations and challenges. Specifically:
-
AI Evaluation Platform - Kradle hosts open-ended, interactive AI "evals" inside Minecraft to benchmark and test adaptive agent behavior
-
Agent-Controlled Environment - AI agents run as web services that receive JSON observations (game events, state, chat) from Kradle and respond with actions (JavaScript code, often using Mineflayer skills)
-
Multi-Agent Challenges - Supports complex scenarios from simple resource collection to multi-agent coordination, PvP/PvE, and custom rule sets
How changes are prepared for the Kradle platform:
Now the extensive integration in the codebase makes perfect sense:
1. Agent Control Interface
- The iframe communication handles AI agent observations and actions
- Status reporting provides real-time agent monitoring for evaluations
- Custom events (
kradle:command,kradle:followPlayer, etc.) are programmatic agent controls
2. Evaluation Infrastructure
- Agent skin mapping for identifying different AI agents in challenges
- Follow system for agent coordination and monitoring
- Replay functionality for recording and analyzing agent behavior
- Connection status reporting for robust agent session management
3. Challenge Environment
- The web client serves as the visual interface for Minecraft-based AI challenges
- Custom proxy (mcproxy.kradle.ai) handles agent connections
- Boss bar integration for challenge progress tracking
So this Minecraft Web Client is essentially the game engine interface that Kradle uses to run its AI agent evaluations in Minecraft worlds. The extensive integration features I identified earlier are all about enabling programmatic control and monitoring of AI agents rather than human players!
🔥 Failed to add learnings
Learnings cannot be added due to server error. Please try again.
| New React components, improve UI (including mobile support). | ||
|
|
||
| ## Workflow | ||
| ## Workflow (from usptream repo. not relevant to Kradle) |
There was a problem hiding this comment.
Fix typos and project reference in section header.
Same issue as above.
Apply:
-## Workflow (from usptream repo. not relevant to Kradle)
+## Workflow (from upstream repo; not specific to this project)📝 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.
| ## Workflow (from usptream repo. not relevant to Kradle) | |
| ## Workflow (from upstream repo; not specific to this project) |
🤖 Prompt for AI Agents
In CONTRIBUTING.md around line 167, the section header contains typos and an
incorrect project reference; change "usptream" to "upstream" and reword the
parenthetical to be clear and grammatically correct (for example: "Workflow
(from upstream repo; not relevant to Kradle)"). Ensure spelling and punctuation
are fixed and that "Kradle" is referenced exactly as intended.
| @@ -0,0 +1 @@ | |||
| <script src="state.ts" type="module"></script> | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Browsers can’t import .ts directly; reference the compiled JS (or ensure a dev server transforms it).
If you’re not running a TS-transforming dev server, this will 404 or load as the wrong MIME type.
Apply this diff (assuming built artifact alongside):
-<script src="state.ts" type="module"></script>
+<script src="state.js" type="module"></script>🏁 Script executed:
#!/bin/bash
# Print the first 20 lines of experiments/state.html to verify the <script> tag
sed -n '1,20p' experiments/state.html
# Check for a compiled state.js alongside state.ts in the same directory
find experiments -maxdepth 1 -type f \( -name "state.js" -o -name "state.ts" \)
# Search entire repo for any state.js artifacts (built or otherwise)
find . -type f -name "state.js"Length of output: 325
Import compiled JS in experiments/state.html
- Change
<script src="state.ts" type="module"></script>to<script src="state.js" type="module"></script>since browsers can’t load.tsmodules without a transformation.
🤖 Prompt for AI Agents
In experiments/state.html around lines 1 to 1, the page currently attempts to
load a TypeScript file directly (<script src="state.ts" type="module">) which
browsers cannot execute; update the script tag to reference the compiled
JavaScript output (state.js) instead and ensure the build process emits
experiments/state.js (or adjust the path to where the compiled file is placed)
so the browser can load the module.
| formattedText: data.motd?.raw ?? '', | ||
| textNameRight: data.online ? | ||
| `${versionClean} ${data.players?.online ?? '??'}/${data.players?.max ?? '??'}` : | ||
| `${versionOverride ?? versionClean} ${data.players?.online ?? '??'}/${data.players?.max ?? '??'}` : |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid rendering “undefined” when version is missing.
Fall back to raw version or “??” if both override and cleaned version are falsy.
- `${versionOverride ?? versionClean} ${data.players?.online ?? '??'}/${data.players?.max ?? '??'}` :
+ `${versionOverride ?? versionClean ?? data.version?.name_raw ?? '??'} ${data.players?.online ?? '??'}/${data.players?.max ?? '??'}` :🤖 Prompt for AI Agents
In src/api/mcStatusApi.ts around line 28, the template currently falls back to
versionClean which can be falsy and may render "undefined"; change the fallback
chain so it uses versionOverride, then versionClean, then the raw version (e.g.,
version or versionRaw) and finally the string '??' if all are falsy—update the
expression to `${versionOverride ?? versionClean ?? version ?? '??'}
${data.players?.online ?? '??'}/${data.players?.max ?? '??'}` (or the actual
raw-version variable name used in this file) so missing version values render a
sensible fallback.
| try { | ||
| audioContext ??= new window.AudioContext() | ||
|
|
||
| const res = await window.fetch(contents) | ||
| if (!res.ok) { | ||
| const error = `Failed to load sound ${path}` | ||
| if (isCypress()) throw new Error(error) | ||
| else console.warn(error) | ||
| return | ||
| } | ||
| const arrayBuffer = await res.arrayBuffer() | ||
|
|
||
| // Decode the audio data immediately | ||
| const audioBuffer = await audioContext.decodeAudioData(arrayBuffer) | ||
| sounds[path] = audioBuffer | ||
| convertedSounds.push(path) // Mark as converted immediately | ||
|
|
||
| loadingSounds.splice(loadingSounds.indexOf(path), 1) | ||
| } catch (err) { | ||
| console.warn(`Failed to load sound ${path}:`, err) | ||
| loadingSounds.splice(loadingSounds.indexOf(path), 1) | ||
| if (isCypress()) throw err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden loader bookkeeping; avoid accidental splice(-1) and clarify return semantics.
Index checks prevent removing the last element when not found; also keep return value semantics clear.
try {
audioContext ??= new window.AudioContext()
const res = await window.fetch(contents)
if (!res.ok) {
const error = `Failed to load sound ${path}`
if (isCypress()) throw new Error(error)
else console.warn(error)
- return
+ const i = loadingSounds.indexOf(path)
+ if (i !== -1) loadingSounds.splice(i, 1)
+ return false
}
const arrayBuffer = await res.arrayBuffer()
// Decode the audio data immediately
const audioBuffer = await audioContext.decodeAudioData(arrayBuffer)
sounds[path] = audioBuffer
convertedSounds.push(path) // Mark as converted immediately
- loadingSounds.splice(loadingSounds.indexOf(path), 1)
+ {
+ const i = loadingSounds.indexOf(path)
+ if (i !== -1) loadingSounds.splice(i, 1)
+ }
+ return true
} catch (err) {
console.warn(`Failed to load sound ${path}:`, err)
- loadingSounds.splice(loadingSounds.indexOf(path), 1)
+ {
+ const i = loadingSounds.indexOf(path)
+ if (i !== -1) loadingSounds.splice(i, 1)
+ }
- if (isCypress()) throw err
+ if (isCypress()) throw err
+ return false
}Follow-up: update the signature to Promise<boolean> and callsites accordingly (see next comment).
📝 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.
| try { | |
| audioContext ??= new window.AudioContext() | |
| const res = await window.fetch(contents) | |
| if (!res.ok) { | |
| const error = `Failed to load sound ${path}` | |
| if (isCypress()) throw new Error(error) | |
| else console.warn(error) | |
| return | |
| } | |
| const arrayBuffer = await res.arrayBuffer() | |
| // Decode the audio data immediately | |
| const audioBuffer = await audioContext.decodeAudioData(arrayBuffer) | |
| sounds[path] = audioBuffer | |
| convertedSounds.push(path) // Mark as converted immediately | |
| loadingSounds.splice(loadingSounds.indexOf(path), 1) | |
| } catch (err) { | |
| console.warn(`Failed to load sound ${path}:`, err) | |
| loadingSounds.splice(loadingSounds.indexOf(path), 1) | |
| if (isCypress()) throw err | |
| } | |
| try { | |
| audioContext ??= new window.AudioContext() | |
| const res = await window.fetch(contents) | |
| if (!res.ok) { | |
| const error = `Failed to load sound ${path}` | |
| if (isCypress()) throw new Error(error) | |
| else console.warn(error) | |
| const i = loadingSounds.indexOf(path) | |
| if (i !== -1) loadingSounds.splice(i, 1) | |
| return false | |
| } | |
| const arrayBuffer = await res.arrayBuffer() | |
| // Decode the audio data immediately | |
| const audioBuffer = await audioContext.decodeAudioData(arrayBuffer) | |
| sounds[path] = audioBuffer | |
| convertedSounds.push(path) // Mark as converted immediately | |
| { | |
| const i = loadingSounds.indexOf(path) | |
| if (i !== -1) loadingSounds.splice(i, 1) | |
| } | |
| return true | |
| } catch (err) { | |
| console.warn(`Failed to load sound ${path}:`, err) | |
| { | |
| const i = loadingSounds.indexOf(path) | |
| if (i !== -1) loadingSounds.splice(i, 1) | |
| } | |
| if (isCypress()) throw err | |
| return false | |
| } |
🤖 Prompt for AI Agents
In src/basicSounds.ts around lines 21 to 43, the loader currently splices
loadingSounds without checking the index and doesn't return an explicit boolean;
update the function to return Promise<boolean> and ensure every control path
returns true on success and false on non-throwing failure. Specifically, capture
const idx = loadingSounds.indexOf(path) and only call loadingSounds.splice(idx,
1) when idx >= 0, and in success return true; on non-ok response or other
recoverable failures return false (but still remove from loadingSounds safely);
in the catch block remove the same guarded-splice, rethrow the error for
isCypress() and otherwise return false. Finally, update all callers to await the
new Promise<boolean> return value and handle the boolean result.
| interface Timer { | ||
| id: number | ||
| callback: () => void | ||
| targetTime: number | ||
| isInterval: boolean | ||
| interval?: number | ||
| cleanup?: () => void | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Track native timer IDs to make clearTimeout/clearInterval effective
Without persisting the native ID, clearTimeout/clearInterval may not cancel the underlying browser timer, causing extra checkTimers() calls.
Apply:
interface Timer {
id: number
callback: () => void
targetTime: number
isInterval: boolean
interval?: number
cleanup?: () => void
+ nativeId?: number
}📝 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.
| interface Timer { | |
| id: number | |
| callback: () => void | |
| targetTime: number | |
| isInterval: boolean | |
| interval?: number | |
| cleanup?: () => void | |
| } | |
| interface Timer { | |
| id: number | |
| callback: () => void | |
| targetTime: number | |
| isInterval: boolean | |
| interval?: number | |
| cleanup?: () => void | |
| nativeId?: number | |
| } |
🤖 Prompt for AI Agents
In src/core/timers.ts around lines 3 to 10, the Timer interface lacks a field to
hold the native browser timer ID so clearTimeout/clearInterval cannot reliably
cancel the underlying timers; add a nativeId?: number (or return type of
setTimeout) to the interface, ensure when scheduling timers you assign the value
from setTimeout/setInterval into timer.nativeId, and when clearing use
clearTimeout(timer.nativeId) or clearInterval(timer.nativeId) (and null out
nativeId) so native timers are actually canceled and prevent extra checkTimers()
runs.
| const timers: Timer[] = [] | ||
|
|
||
| // TODO implementation breaks tps (something is wrong with intervals) | ||
| const fixBrowserTimers = () => { |
There was a problem hiding this comment.
Export the monkey-patcher
fixBrowserTimers isn’t exported, so patching can’t be enabled by callers.
Apply:
-const fixBrowserTimers = () => {
+export const fixBrowserTimers = () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fixBrowserTimers = () => { | |
| export const fixBrowserTimers = () => { |
🤖 Prompt for AI Agents
In src/core/timers.ts around line 16, the function fixBrowserTimers is declared
but not exported so callers cannot enable the monkey-patch; change it to a named
export (e.g. export const fixBrowserTimers = () => { ... }) and, if your project
uses a barrel file, add this symbol to the appropriate export surface (index.ts
or equivalent) so consumers can import and call it.
| export default (markdown: string) => { | ||
| const arr = markdown.split('\n\n') | ||
| const lines = ['', '', '', ''] | ||
| const lines = ['', '', '', ''] as any[] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop erasing types with any[]; introduce explicit MC text component types and type the return.
Casting to any[] hides real issues and weakens safety at the module boundary. Define a concrete text component type and make lines strongly typed.
Apply this diff:
@@
-import { remark } from 'remark'
+import { remark } from 'remark'
+
+type MCComponent = { text: string; bold?: boolean; italic?: boolean }
+type MCLine = MCComponent[]
@@
-export default (markdown: string) => {
+export default (markdown: string): Array<MCLine | ''> => {
@@
- const lines = ['', '', '', ''] as any[]
+ const lines: Array<MCLine | ''> = ['', '', '', '']📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lines = ['', '', '', ''] as any[] | |
| import { remark } from 'remark' | |
| type MCComponent = { text: string; bold?: boolean; italic?: boolean } | |
| type MCLine = MCComponent[] | |
| export default (markdown: string): Array<MCLine | ''> => { | |
| const lines: Array<MCLine | ''> = ['', '', '', ''] | |
| // …rest of your implementation… | |
| } |
🤖 Prompt for AI Agents
In src/markdownToFormattedText.ts around line 5, the code uses "const lines =
['', '', '', ''] as any[]" which erases types; replace this with a concrete MC
text component type (e.g., a type/ interface describing the shape of the text
component objects used in this module) and declare lines with that type (e.g.,
const lines: MCTextComponent[] = [...]) and update the function's return type to
the appropriate array/structure of MCTextComponent instead of implicit any;
ensure all push/assignment sites conform to the new type and import or export
the MCTextComponent type where needed.
| - uses: peaceiris/actions-gh-pages@v3 | ||
| with: | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} | ||
| publish_dir: .vercel/output/static | ||
| force_orphan: true | ||
|
|
There was a problem hiding this comment.
Update GH Pages action to v4.
actionlint flags v3 as too old for current runners.
- - uses: peaceiris/actions-gh-pages@v3
+ - uses: peaceiris/actions-gh-pages@v4
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
publish_dir: .vercel/output/static
force_orphan: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: peaceiris/actions-gh-pages@v3 | |
| with: | |
| github_token: ${{ secrets.GITHUB_TOKEN }} | |
| publish_dir: .vercel/output/static | |
| force_orphan: true | |
| - uses: peaceiris/actions-gh-pages@v4 | |
| with: | |
| github_token: ${{ secrets.GITHUB_TOKEN }} | |
| publish_dir: .vercel/output/static | |
| force_orphan: true |
🧰 Tools
🪛 actionlint (1.7.7)
50-50: the runner of "peaceiris/actions-gh-pages@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/publish.yml around lines 50 to 55, the workflow uses
peaceiris/actions-gh-pages@v3 which actionlint flags as outdated; update the
action reference to peaceiris/actions-gh-pages@v4 (i.e., change the uses line to
@v4) while preserving the existing inputs (github_token, publish_dir,
force_orphan) and verify there are no breaking input changes in v4; run a local
lint or dry-run CI to ensure the updated action works with current runners.
| - name: Build single-file version - minecraft.html | ||
| run: pnpm build-single-file && mv dist/single/index.html minecraft.html | ||
| - name: Build self-host version | ||
| run: pnpm build | ||
| - name: Bundle server.js | ||
| run: | | ||
| pnpm esbuild server.js --bundle --platform=node --outfile=bundled-server.js --define:process.env.NODE_ENV="'production'" | ||
| - name: Create zip package | ||
| run: | | ||
| mkdir -p package | ||
| cp -r dist package/ | ||
| cp bundled-server.js package/server.js | ||
| cd package | ||
| zip -r ../self-host.zip . | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Install project deps before local build/package steps.
These steps will fail without pnpm install (the earlier install is commented out).
+ - name: Install dependencies
+ run: pnpm install --frozen-lockfile
+
- name: Build single-file version - minecraft.html
run: pnpm build-single-file && mv dist/single/index.html minecraft.html
- name: Build self-host version
run: pnpm buildOptional: enable caching for faster runs:
- add to actions/setup-node:
cache: 'pnpm'.
📝 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.
| - name: Build single-file version - minecraft.html | |
| run: pnpm build-single-file && mv dist/single/index.html minecraft.html | |
| - name: Build self-host version | |
| run: pnpm build | |
| - name: Bundle server.js | |
| run: | | |
| pnpm esbuild server.js --bundle --platform=node --outfile=bundled-server.js --define:process.env.NODE_ENV="'production'" | |
| - name: Create zip package | |
| run: | | |
| mkdir -p package | |
| cp -r dist package/ | |
| cp bundled-server.js package/server.js | |
| cd package | |
| zip -r ../self-host.zip . | |
| - name: Install dependencies | |
| run: pnpm install --frozen-lockfile | |
| - name: Build single-file version - minecraft.html | |
| run: pnpm build-single-file && mv dist/single/index.html minecraft.html | |
| - name: Build self-host version | |
| run: pnpm build | |
| - name: Bundle server.js | |
| run: | | |
| pnpm esbuild server.js --bundle --platform=node --outfile=bundled-server.js --define:process.env.NODE_ENV="'production'" | |
| - name: Create zip package | |
| run: | | |
| mkdir -p package | |
| cp -r dist package/ | |
| cp bundled-server.js package/server.js | |
| cd package | |
| zip -r ../self-host.zip . |
🤖 Prompt for AI Agents
In .github/workflows/publish.yml around lines 56 to 71, the workflow runs pnpm
build and other local build/packaging steps without installing dependencies (the
earlier install is commented out), causing these steps to fail; add a step
before the build steps to run pnpm install (preferably pnpm install
--frozen-lockfile) so dependencies are available, and optionally re-enable
caching by configuring actions/setup-node with cache: 'pnpm' to speed up runs.
| @@ -1,7 +1,8 @@ | |||
| //@ts-nocheck | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid //@ts-nocheck; declare globals instead
Replace ts-nocheck with explicit global declarations to keep type safety.
Apply this diff:
-//@ts-nocheck
+// Declare globals provided by playground runtime
+declare const viewer: any
+declare const worldView: any🤖 Prompt for AI Agents
In renderer/playground/scenes/lightingStarfield.ts around line 1, remove the
top-level "//@ts-nocheck" and instead add explicit TypeScript global
declarations for any ambient globals used in this file (e.g., THREE,
requestAnimationFrame, window, document, or any project-specific globals).
Identify which undeclared globals the file references, create a declaration
block (or add to an existing .d.ts) declaring their types, and import specific
types/modules where possible rather than relying on globals. Ensure the file
compiles without ts-nocheck by iteratively fixing remaining type errors (prefer
typed imports over any-typed globals).
| import { GameMode } from 'mineflayer' | ||
| import { HandItemBlock } from '../three/holdingBlock' | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import GameMode as a type to avoid bundling runtime dependency in renderer
Renderer code should avoid pulling mineflayer at runtime.
Apply:
-import { GameMode } from 'mineflayer'
+import type { GameMode } from 'mineflayer'📝 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.
| import { GameMode } from 'mineflayer' | |
| import { HandItemBlock } from '../three/holdingBlock' | |
| import type { GameMode } from 'mineflayer' | |
| import { HandItemBlock } from '../three/holdingBlock' |
🤖 Prompt for AI Agents
In renderer/viewer/lib/basePlayerState.ts around lines 6 to 8, the GameMode
import pulls in the mineflayer runtime into the renderer bundle; change the
import to a TypeScript type-only import so GameMode is erased at emit time
(e.g., use an "import type" for GameMode) and keep the HandItemBlock import
as-is to avoid adding mineflayer to the renderer runtime.
| reactive = proxy({ | ||
| playerSkin: undefined | ||
| playerSkin: undefined as string | undefined, | ||
| inWater: false, | ||
| backgroundColor: [0, 0, 0] as [number, number, number], | ||
| ambientLight: 0, | ||
| directionalLight: 0, | ||
| }) |
There was a problem hiding this comment.
Include gameMode in reactive defaults
You subscribe to reactive.gameMode elsewhere; ensure the key exists on the proxy.
Apply:
reactive = proxy({
playerSkin: undefined as string | undefined,
inWater: false,
backgroundColor: [0, 0, 0] as [number, number, number],
ambientLight: 0,
directionalLight: 0,
+ gameMode: undefined as GameMode | 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.
| reactive = proxy({ | |
| playerSkin: undefined | |
| playerSkin: undefined as string | undefined, | |
| inWater: false, | |
| backgroundColor: [0, 0, 0] as [number, number, number], | |
| ambientLight: 0, | |
| directionalLight: 0, | |
| }) | |
| reactive = proxy({ | |
| playerSkin: undefined as string | undefined, | |
| inWater: false, | |
| backgroundColor: [0, 0, 0] as [number, number, number], | |
| ambientLight: 0, | |
| directionalLight: 0, | |
| gameMode: undefined as GameMode | undefined, | |
| }) |
🤖 Prompt for AI Agents
In renderer/viewer/lib/basePlayerState.ts around lines 45 to 51, the reactive
proxy is missing the gameMode key which causes subscribers to reactive.gameMode
to fail; add a gameMode property to the proxy defaults (e.g., gameMode:
undefined as string | undefined or a suitable enum/default value) so the key
exists on the reactive object and subscriptions can read/update it safely.
| bot._client.registerChannel(CHANNEL_NAME, packetStructure, true) | ||
|
|
||
| // Trigger update | ||
| const block = worldView!.world.getBlock(new Vec3(x, y, z)) | ||
| if (block) { | ||
| worldView!.world.setBlockStateId(new Vec3(x, y, z), block.stateId) | ||
| } | ||
| bot._client.on(CHANNEL_NAME as any, (data) => { | ||
| const { id, categoryTitle, items } = data | ||
| if (items === '') { | ||
| // remove category | ||
| jeiCustomCategories.value = jeiCustomCategories.value.filter(x => x.id !== id) | ||
| return | ||
| } | ||
| const PrismarineItem = PItem(bot.version) | ||
| jeiCustomCategories.value.push({ |
There was a problem hiding this comment.
JEI channel: field name mismatch (categoryTitle) and potential duplicates.
packetStructure defines _categoryTitle, but handler destructures categoryTitle. Fix the structure or destructuring. Also, avoid duplicate categories by id.
- {
- name: '_categoryTitle',
- type: ['pstring', { countType: 'i16' }]
- },
+ {
+ name: 'categoryTitle',
+ type: ['pstring', { countType: 'i16' }]
+ },And before pushing:
- jeiCustomCategories.value.push({
+ jeiCustomCategories.value = jeiCustomCategories.value.filter(x => x.id !== id)
+ jeiCustomCategories.value.push({📝 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.
| bot._client.registerChannel(CHANNEL_NAME, packetStructure, true) | |
| // Trigger update | |
| const block = worldView!.world.getBlock(new Vec3(x, y, z)) | |
| if (block) { | |
| worldView!.world.setBlockStateId(new Vec3(x, y, z), block.stateId) | |
| } | |
| bot._client.on(CHANNEL_NAME as any, (data) => { | |
| const { id, categoryTitle, items } = data | |
| if (items === '') { | |
| // remove category | |
| jeiCustomCategories.value = jeiCustomCategories.value.filter(x => x.id !== id) | |
| return | |
| } | |
| const PrismarineItem = PItem(bot.version) | |
| jeiCustomCategories.value.push({ | |
| // … earlier in src/customChannels.ts … | |
| const packetStructure = [ | |
| { name: 'id', type: 'i32' }, | |
| - { | |
| - name: '_categoryTitle', | |
| - type: ['pstring', { countType: 'i16' }] | |
| { | |
| name: 'categoryTitle', | |
| type: ['pstring', { countType: 'i16' }] | |
| }, | |
| { name: 'items', type: ['pstring', { countType: 'i16' }] }, | |
| // … any other fields … | |
| ] | |
| // … later in the same file … | |
| bot._client.registerChannel(CHANNEL_NAME, packetStructure, true) | |
| bot._client.on(CHANNEL_NAME as any, (data) => { | |
| const { id, categoryTitle, items } = data | |
| if (items === '') { | |
| // remove category | |
| jeiCustomCategories.value = jeiCustomCategories.value.filter(x => x.id !== id) | |
| return | |
| } | |
| const PrismarineItem = PItem(bot.version) | |
| // ensure we don’t accumulate duplicates for the same id | |
| jeiCustomCategories.value = jeiCustomCategories.value.filter(x => x.id !== id) | |
| jeiCustomCategories.value.push({ | |
| id, | |
| categoryTitle, | |
| items: items.map(item => new PrismarineItem(item)) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In src/customChannels.ts around lines 96 to 106 the JEI channel handler
mismatches the packet field name (_categoryTitle vs categoryTitle) and can push
duplicate categories by id; fix by using the same field name the packetStructure
defines (use _categoryTitle when destructuring or update packetStructure to
categoryTitle), and before pushing new category check for an existing entry with
the same id and replace it (or skip push) — i.e., find index by id and update
that element, otherwise push the new category.
| const { replayFileUrl } = appQueryParams | ||
| if (replayFileUrl) { | ||
| setLoadingScreenStatus('Downloading replay file') | ||
| const response = await fetch(replayFileUrl) | ||
| const contentLength = response.headers?.get('Content-Length') | ||
| const size = contentLength ? +contentLength : undefined | ||
| const filename = replayFileUrl.split('/').pop() | ||
|
|
||
| let downloadedBytes = 0 | ||
| const buffer = await new Response(new ReadableStream({ | ||
| async start (controller) { | ||
| if (!response.body) throw new Error('Server returned no response!') | ||
| const reader = response.body.getReader() | ||
|
|
||
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { | ||
| const { done, value } = await reader.read() | ||
|
|
||
| if (done) { | ||
| controller.close() | ||
| break | ||
| } | ||
|
|
||
| downloadedBytes += value.byteLength | ||
|
|
||
| // Calculate download progress as a percentage | ||
| const progress = size ? (downloadedBytes / size) * 100 : undefined | ||
| setLoadingScreenStatus(`Download replay file progress: ${progress === undefined ? '?' : Math.floor(progress)}% (${getFixedFilesize(downloadedBytes)} / ${size && getFixedFilesize(size)})`, false, true) | ||
|
|
||
| // Pass the received data to the controller | ||
| controller.enqueue(value) | ||
| } | ||
| }, | ||
| })).arrayBuffer() | ||
|
|
||
| // Convert buffer to text, handling any compression automatically | ||
| const decoder = new TextDecoder() | ||
| const contents = decoder.decode(buffer) | ||
|
|
||
| openFile({ | ||
| contents, | ||
| filename, | ||
| filesize: size | ||
| }) | ||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden download path: check response.ok, clean progress text, validate replay, safe filename fallback.
- Check response.ok before reading body.
- Avoid “undefined/false” in progress strings when Content-Length is missing.
- Use VALID_REPLAY_EXTENSIONS to guard replay loads.
- Provide a filename fallback if URL has no tail.
- const { replayFileUrl } = appQueryParams
+ const { replayFileUrl } = appQueryParams
if (replayFileUrl) {
setLoadingScreenStatus('Downloading replay file')
- const response = await fetch(replayFileUrl)
+ const response = await fetch(replayFileUrl)
+ if (!response.ok) throw new Error(`HTTP ${response.status} ${response.statusText}`)
const contentLength = response.headers?.get('Content-Length')
const size = contentLength ? +contentLength : undefined
- const filename = replayFileUrl.split('/').pop()
+ const rawName = replayFileUrl.split('/').pop() || ''
+ const filename = rawName || 'replay.txt'
+ if (!VALID_REPLAY_EXTENSIONS?.some(ext => filename.endsWith(ext))) {
+ throw new Error(`Unsupported replay extension for "${filename}"`)
+ }
@@
- const progress = size ? (downloadedBytes / size) * 100 : undefined
- setLoadingScreenStatus(`Download replay file progress: ${progress === undefined ? '?' : Math.floor(progress)}% (${getFixedFilesize(downloadedBytes)} / ${size && getFixedFilesize(size)})`, false, true)
+ const progress = size ? (downloadedBytes / size) * 100 : undefined
+ const totalStr = size ? getFixedFilesize(size) : 'unknown'
+ setLoadingScreenStatus(`Download replay file progress: ${progress === undefined ? '?' : Math.floor(progress)}% (${getFixedFilesize(downloadedBytes)} / ${totalStr})`, false, true)- setLoadingScreenStatus(`Downloading ${downloadThing} ${name}: have to download ${contentLength && getFixedFilesize(contentLength)}...`)
+ setLoadingScreenStatus(`Downloading ${downloadThing} ${name}: have to download ${contentLength ? getFixedFilesize(contentLength) : 'unknown'}...`)
@@
- const progress = contentLength ? (downloadedBytes / contentLength) * 100 : undefined
- setLoadingScreenStatus(`Download ${downloadThing} progress: ${progress === undefined ? '?' : Math.floor(progress)}% (${getFixedFilesize(downloadedBytes)} / ${contentLength && getFixedFilesize(contentLength)})`, false, true)
+ const progress = contentLength ? (downloadedBytes / contentLength) * 100 : undefined
+ const totalStr = contentLength ? getFixedFilesize(contentLength) : 'unknown'
+ setLoadingScreenStatus(`Download ${downloadThing} progress: ${progress === undefined ? '?' : Math.floor(progress)}% (${getFixedFilesize(downloadedBytes)} / ${totalStr})`, false, true)Note: The comment “handling any compression automatically” is misleading—TextDecoder won’t decompress. If replays can be compressed, detect content-encoding and decompress before decode.
Also applies to: 95-116
🤖 Prompt for AI Agents
In src/downloadAndOpenFile.ts around lines 14 to 59 (and similarly apply fixes
to 95-116), first verify response.ok and throw or set an error state if false
before reading response.body; parse Content-Encoding header and if compressed
(gzip/deflate/br) run appropriate decompression on the response bytes before
decoding (or remove the misleading comment if compression isn't expected);
compute progress text defensively so it never interpolates "undefined" or
"false" (e.g., show "?" or "unknown" and only show the size portion when
Content-Length is present); validate the filename by extracting the URL tail and
falling back to a safe default like "replay.bin" and ensure its extension
matches VALID_REPLAY_EXTENSIONS before calling openFile (reject or warn
otherwise).
| await installResourcepackPack(buffer, createFullScreenProgressReporter(), name) | ||
| } else { |
There was a problem hiding this comment.
Bug: wrong installResourcepackPack argument order installs every pack into 'default'.
installResourcepackPack(file, progressReporter, displayName, name, isServer) — you’re passing only 3 args, so name defaults to "default". Pass the pack folder name as the 4th arg.
Apply this fix:
- await installResourcepackPack(buffer, createFullScreenProgressReporter(), name)
+ // displayName = name, install folder name = name
+ await installResourcepackPack(buffer, createFullScreenProgressReporter(), name, name)📝 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.
| await installResourcepackPack(buffer, createFullScreenProgressReporter(), name) | |
| } else { | |
| // displayName = name, install folder name = name | |
| await installResourcepackPack(buffer, createFullScreenProgressReporter(), name, name) | |
| } else { |
🤖 Prompt for AI Agents
In src/downloadAndOpenFile.ts around lines 126 to 127, installResourcepackPack
is being called with only 3 arguments causing the pack folder name parameter to
default to "default"; update the call to pass the pack folder name as the 4th
argument (i.e., installResourcepackPack(buffer,
createFullScreenProgressReporter(), name, packFolderName) or the appropriate
variable that holds the pack folder) so the correct folder is used instead of
"default".
| if (VALID_REPLAY_EXTENSIONS.some(ext => file.name.endsWith(ext)) || file.name.startsWith('packets-replay')) { | ||
| const contents = await file.text() | ||
| openFile({ | ||
| contents, | ||
| filename: file.name, | ||
| filesize: file.size | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make replay detection case-insensitive and await openFile.
Prevents false negatives for mixed-case filenames and avoids unhandled promise rejections.
- if (VALID_REPLAY_EXTENSIONS.some(ext => file.name.endsWith(ext)) || file.name.startsWith('packets-replay')) {
- const contents = await file.text()
- openFile({
+ const nameLc = file.name.toLowerCase()
+ if (VALID_REPLAY_EXTENSIONS.some(ext => nameLc.endsWith(ext.toLowerCase())) || nameLc.startsWith('packets-replay')) {
+ const contents = await file.text()
+ await openFile({
contents,
filename: file.name,
filesize: file.size
})
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (VALID_REPLAY_EXTENSIONS.some(ext => file.name.endsWith(ext)) || file.name.startsWith('packets-replay')) { | |
| const contents = await file.text() | |
| openFile({ | |
| contents, | |
| filename: file.name, | |
| filesize: file.size | |
| }) | |
| return | |
| } | |
| const nameLc = file.name.toLowerCase() | |
| if ( | |
| VALID_REPLAY_EXTENSIONS.some(ext => nameLc.endsWith(ext.toLowerCase())) || | |
| nameLc.startsWith('packets-replay') | |
| ) { | |
| const contents = await file.text() | |
| await openFile({ | |
| contents, | |
| filename: file.name, | |
| filesize: file.size | |
| }) | |
| return | |
| } |
🤖 Prompt for AI Agents
In src/dragndrop.ts around lines 57 to 65, the replay-file check is
case-sensitive and openFile is not awaited; update it to use a lower-cased
filename variable (e.g., const name = file.name.toLowerCase()) and compare
against VALID_REPLAY_EXTENSIONS using name.endsWith(ext) and
name.startsWith('packets-replay'), and call await openFile({...}) so the promise
is awaited before returning.
| const tempPath = '/temp/temp.mca' | ||
| try { | ||
| await fs.promises.writeFile(tempPath, Buffer.from(await file.arrayBuffer())) | ||
| await fs.promises.writeFile(tempPath, Buffer.from(await file.arrayBuffer()) as any) | ||
| const region = new RegionFile(tempPath) |
There was a problem hiding this comment.
Ensure temp dir exists, use typed write, and make cleanup robust.
Avoids ENOENT on mkdir/unlink and removes the any-cast.
- const tempPath = '/temp/temp.mca'
+ const tempDir = '/temp'
+ const tempPath = `${tempDir}/temp.mca`
try {
- await fs.promises.writeFile(tempPath, Buffer.from(await file.arrayBuffer()) as any)
+ await fs.promises.mkdir(tempDir, { recursive: true })
+ const ab = await file.arrayBuffer()
+ await fs.promises.writeFile(tempPath, Buffer.from(new Uint8Array(ab)))
const region = new RegionFile(tempPath)
@@
- await fs.promises.unlink(tempPath)
+ await fs.promises.unlink(tempPath).catch(() => {})Also applies to: 106-106
🤖 Prompt for AI Agents
In src/dragndrop.ts around lines 67-70 and line 106, ensure the directory for
tempPath exists before writing (use fs.promises.mkdir(path.dirname(tempPath), {
recursive: true })), stop using the any cast by passing a proper Buffer to
writeFile (Buffer.from(await file.arrayBuffer())), and make cleanup robust by
surrounding the RegionFile usage with try/finally where you attempt to
fs.promises.unlink(tempPath) inside a try/catch that ignores ENOENT; also
consider using path.join or a unique tmp filename instead of a hard-coded
'/temp/temp.mca'.
| export const activeGuiAtlas = proxy({ | ||
| atlas: null as null | { json, image }, | ||
| }) |
There was a problem hiding this comment.
Type error: invalid inline type for activeGuiAtlas.atlas.
{ json, image } is not a valid type literal; both properties need explicit types. This will fail TS checks.
Apply this diff:
-export const activeGuiAtlas = proxy({
- atlas: null as null | { json, image },
-})
+type GuiAtlas = { json: any; image: HTMLImageElement }
+export const activeGuiAtlas = proxy<{ atlas: GuiAtlas | null }>({
+ atlas: null,
+})📝 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 const activeGuiAtlas = proxy({ | |
| atlas: null as null | { json, image }, | |
| }) | |
| // Define a proper type for the GUI atlas | |
| type GuiAtlas = { | |
| json: any | |
| image: HTMLImageElement | |
| } | |
| export const activeGuiAtlas = proxy<{ atlas: GuiAtlas | null }>({ | |
| atlas: null, | |
| }) |
🤖 Prompt for AI Agents
In renderer/viewer/lib/guiRenderer.ts around lines 12-14, the inline type `{
json, image }` is invalid TypeScript; replace it with an explicit type (either
inline like `null | { json: any; image: any }` or by declaring an interface/type
alias such as `type GuiAtlas = { json: any; image: HTMLImageElement | string |
ImageBitmap }` and using `atlas: null as null | GuiAtlas`) so both `json` and
`image` have explicit types; update the proxy initialization accordingly.
| if (block.models === undefined && blockProvider) { | ||
| if (!attr) throw new Error('attr is required') | ||
| const props = block.getProperties() |
There was a problem hiding this comment.
Runtime contract mismatch: attr is optional in the signature but required at runtime.
Throwing when attr is missing contradicts the type and can break existing callers.
Apply this diff:
- if (block.models === undefined && blockProvider) {
- if (!attr) throw new Error('attr is required')
+ if (block.models === undefined && blockProvider) {
+ attr ??= {}🤖 Prompt for AI Agents
In renderer/viewer/lib/mesher/world.ts around lines 169-171, the code throws
when attr is missing even though the function signature marks attr as optional;
change the runtime to match the contract by removing the throw and handling
undefined attr (e.g., default attr to an empty object or otherwise handle the
absent attr path before calling block.getProperties()), or alternatively make
attr required in the function signature if that better matches callers — ensure
no runtime throw occurs for a legitimately-optional attr.
| if (!this.sentBlockStateModels.has(cacheKey)) { | ||
| this.blockStateModelInfo.set(cacheKey, { | ||
| cacheKey, | ||
| issues, | ||
| modelNames: resolvedModelNames, | ||
| conditions: resolvedConditions | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
sentBlockStateModels is never populated; tracking won’t work.
You has(cacheKey) but never add(cacheKey), so the info is recomputed each time.
Apply this diff:
if (!this.sentBlockStateModels.has(cacheKey)) {
this.blockStateModelInfo.set(cacheKey, {
cacheKey,
issues,
modelNames: resolvedModelNames,
conditions: resolvedConditions
})
+ this.sentBlockStateModels.add(cacheKey)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!this.sentBlockStateModels.has(cacheKey)) { | |
| this.blockStateModelInfo.set(cacheKey, { | |
| cacheKey, | |
| issues, | |
| modelNames: resolvedModelNames, | |
| conditions: resolvedConditions | |
| }) | |
| } | |
| if (!this.sentBlockStateModels.has(cacheKey)) { | |
| this.blockStateModelInfo.set(cacheKey, { | |
| cacheKey, | |
| issues, | |
| modelNames: resolvedModelNames, | |
| conditions: resolvedConditions | |
| }) | |
| this.sentBlockStateModels.add(cacheKey) | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/lib/mesher/world.ts around lines 203 to 211,
sentBlockStateModels is checked with has(cacheKey) but never populated, so the
cache guard is ineffective; after you set blockStateModelInfo for the new
cacheKey, add this.sentBlockStateModels.add(cacheKey) to mark it as sent (ensure
sentBlockStateModels is initialized as a Set elsewhere), so subsequent calls
will skip recomputation.
| getItemMesh (item, specificProps: ItemSpecificContextProperties, previousModel?: string) { | ||
| if (!item.nbt && item.nbtData) item.nbt = item.nbtData | ||
| const textureUv = this.worldRenderer.getItemRenderData(item, specificProps) | ||
| if (previousModel && previousModel === textureUv?.modelName) return undefined | ||
|
|
||
| if (textureUv && 'resolvedModel' in textureUv) { | ||
| const mesh = getBlockMeshFromModel(this.viewer.world.material, textureUv.resolvedModel, textureUv.modelName) | ||
| const mesh = getBlockMeshFromModel(this.worldRenderer.material, textureUv.resolvedModel, textureUv.modelName, this.worldRenderer.resourcesManager.currentResources!.worldBlockProvider) | ||
| let SCALE = 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid redundant model rebuilds and pass loadedData explicitly.
Good early-return on same model. Also, fix the call to getBlockMeshFromModel to pass loadedData (matches proposed signature).
- const mesh = getBlockMeshFromModel(this.worldRenderer.material, textureUv.resolvedModel, textureUv.modelName, this.worldRenderer.resourcesManager.currentResources!.worldBlockProvider)
+ const mesh = getBlockMeshFromModel(
+ this.worldRenderer.material,
+ textureUv.resolvedModel,
+ textureUv.modelName,
+ this.worldRenderer.resourcesManager.currentResources!.worldBlockProvider,
+ this.worldRenderer.resourcesManager.currentResources!.loadedData
+ )📝 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.
| getItemMesh (item, specificProps: ItemSpecificContextProperties, previousModel?: string) { | |
| if (!item.nbt && item.nbtData) item.nbt = item.nbtData | |
| const textureUv = this.worldRenderer.getItemRenderData(item, specificProps) | |
| if (previousModel && previousModel === textureUv?.modelName) return undefined | |
| if (textureUv && 'resolvedModel' in textureUv) { | |
| const mesh = getBlockMeshFromModel(this.viewer.world.material, textureUv.resolvedModel, textureUv.modelName) | |
| const mesh = getBlockMeshFromModel(this.worldRenderer.material, textureUv.resolvedModel, textureUv.modelName, this.worldRenderer.resourcesManager.currentResources!.worldBlockProvider) | |
| let SCALE = 1 | |
| getItemMesh (item, specificProps: ItemSpecificContextProperties, previousModel?: string) { | |
| if (!item.nbt && item.nbtData) item.nbt = item.nbtData | |
| const textureUv = this.worldRenderer.getItemRenderData(item, specificProps) | |
| if (previousModel && previousModel === textureUv?.modelName) return undefined | |
| if (textureUv && 'resolvedModel' in textureUv) { | |
| const mesh = getBlockMeshFromModel( | |
| this.worldRenderer.material, | |
| textureUv.resolvedModel, | |
| textureUv.modelName, | |
| this.worldRenderer.resourcesManager.currentResources!.worldBlockProvider, | |
| this.worldRenderer.resourcesManager.currentResources!.loadedData | |
| ) | |
| let SCALE = 1 |
🤖 Prompt for AI Agents
In renderer/viewer/three/entities.ts around lines 506 to 513, the code correctly
early-returns when previousModel matches textureUv.modelName but the call to
getBlockMeshFromModel is missing the new loadedData argument; update the call to
pass textureUv.loadedData (or textureUv?.loadedData) as the next parameter after
resolvedModel, keep the existing early-return check intact to avoid redundant
rebuilds, and ensure types/null checks align so loadedData is passed only when
present.
| const startWorld = (displayOptions: DisplayWorldOptions) => { | ||
| if (panoramaRenderer) { | ||
| panoramaRenderer.dispose() | ||
| panoramaRenderer = null | ||
| } | ||
| worldRenderer = new WorldRendererThree(documentRenderer.renderer, initOptions, displayOptions) | ||
| documentRenderer.render = (sizeChanged: boolean) => { | ||
| worldRenderer?.render(sizeChanged) | ||
| } | ||
| window.world = worldRenderer | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against multiple world starts; dispose an existing world first.
Starting a new world without tearing down the old one can leak GPU/DOM resources.
Apply this diff:
- const startWorld = (displayOptions: DisplayWorldOptions) => {
+ const startWorld = (displayOptions: DisplayWorldOptions) => {
if (panoramaRenderer) {
panoramaRenderer.dispose()
panoramaRenderer = null
}
+ if (worldRenderer) {
+ worldRenderer.destroy()
+ worldRenderer = null
+ }
worldRenderer = new WorldRendererThree(documentRenderer.renderer, initOptions, displayOptions)
documentRenderer.render = (sizeChanged: boolean) => {
worldRenderer?.render(sizeChanged)
}
window.world = worldRenderer
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startWorld = (displayOptions: DisplayWorldOptions) => { | |
| if (panoramaRenderer) { | |
| panoramaRenderer.dispose() | |
| panoramaRenderer = null | |
| } | |
| worldRenderer = new WorldRendererThree(documentRenderer.renderer, initOptions, displayOptions) | |
| documentRenderer.render = (sizeChanged: boolean) => { | |
| worldRenderer?.render(sizeChanged) | |
| } | |
| window.world = worldRenderer | |
| } | |
| const startWorld = (displayOptions: DisplayWorldOptions) => { | |
| if (panoramaRenderer) { | |
| panoramaRenderer.dispose() | |
| panoramaRenderer = null | |
| } | |
| if (worldRenderer) { | |
| worldRenderer.destroy() | |
| worldRenderer = null | |
| } | |
| worldRenderer = new WorldRendererThree(documentRenderer.renderer, initOptions, displayOptions) | |
| documentRenderer.render = (sizeChanged: boolean) => { | |
| worldRenderer?.render(sizeChanged) | |
| } | |
| window.world = worldRenderer | |
| } |
🤖 Prompt for AI Agents
In renderer/viewer/three/graphicsBackend.ts around lines 62 to 73, starting a
new world currently creates a new WorldRendererThree even if one already exists,
leaking GPU/DOM resources; modify startWorld to check if worldRenderer exists
and if so call its dispose() and set it to null (and also clear window.world)
before creating the new WorldRendererThree, then assign documentRenderer.render
to call the new renderer and set window.world to the new instance; ensure any
associated cleanup (removing event listeners, canceling animation frames) in
WorldRendererThree.dispose is relied upon so no other teardown is required here.
| appViewer.lastCamUpdate = Date.now() | ||
| setThirdPersonCamera() | ||
| void appViewer.worldView?.updatePosition(following.entity.position) | ||
| } |
There was a problem hiding this comment.
Null-safety: guard before dereferencing following.entity.
If following is unset (or lost), this will throw.
- setThirdPersonCamera()
- void appViewer.worldView?.updatePosition(following.entity.position)
+ setThirdPersonCamera()
+ if (following?.entity?.position) {
+ void appViewer.worldView?.updatePosition(following.entity.position)
+ }📝 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.
| appViewer.lastCamUpdate = Date.now() | |
| setThirdPersonCamera() | |
| void appViewer.worldView?.updatePosition(following.entity.position) | |
| } | |
| appViewer.lastCamUpdate = Date.now() | |
| setThirdPersonCamera() | |
| if (following?.entity?.position) { | |
| void appViewer.worldView?.updatePosition(following.entity.position) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/follow.ts around lines 19 to 22, the code dereferences following.entity
without a null check which can throw if following is unset; before using
following.entity.position, add a guard (e.g., if (!following ||
!following.entity || !following.entity.position) return) or use optional
chaining to abort early, then only call setThirdPersonCamera and
appViewer.worldView?.updatePosition with the confirmed position; ensure types
are narrowed so the compiler knows following.entity.position is defined.
| export async function setFollowingPlayer (username?: string) { | ||
| if (username && bot.players[username]) { | ||
| // start following player | ||
| console.log(`Following player '${username}'`) | ||
|
|
||
| // tell the watcher to keep us in range of the target player | ||
| // via teleporting to the target player | ||
| bot.whisper('watcher', `follow ${username}`) | ||
|
|
||
|
|
||
| let target = bot.players[username] |
There was a problem hiding this comment.
Critical: following a player short-circuits when player entry isn’t loaded yet.
if (username && bot.players[username]) sends you to the “unfollow/self” branch when the player isn’t in bot.players yet. You intended to wait; remove the bot.players[username] guard in the top-level condition.
-export async function setFollowingPlayer (username?: string) {
- if (username && bot.players[username]) {
+export async function setFollowingPlayer (username?: string) {
+ if (username) {Rest of the logic already retries until the target appears.
📝 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 async function setFollowingPlayer (username?: string) { | |
| if (username && bot.players[username]) { | |
| // start following player | |
| console.log(`Following player '${username}'`) | |
| // tell the watcher to keep us in range of the target player | |
| // via teleporting to the target player | |
| bot.whisper('watcher', `follow ${username}`) | |
| let target = bot.players[username] | |
| export async function setFollowingPlayer (username?: string) { | |
| if (username) { | |
| // start following player | |
| console.log(`Following player '${username}'`) | |
| // tell the watcher to keep us in range of the target player | |
| // via teleporting to the target player | |
| bot.whisper('watcher', `follow ${username}`) | |
| let target = bot.players[username] | |
| // ... rest of logic that retries until target appears |
🤖 Prompt for AI Agents
In src/follow.ts around lines 91-101, the top-level condition short-circuits
when bot.players[username] is not yet populated; change the guard from "if
(username && bot.players[username])" to just "if (username)" so we don't take
the unfollow/self branch prematurely, and ensure any direct access to
bot.players[username] (e.g. the let target = bot.players[username] assignment)
is moved inside or after the existing retry/wait loop so we only read
bot.players[username] once it exists.
| window.addEventListener('message', (event) => { | ||
| const { data } = event | ||
| if (data.source === 'kradle-frontend') { | ||
| console.log('[iframe-rpc] [minecraft-web-client] Received message', data) | ||
| customEvents.emit(`kradle:${data.action as ReceivableActions}`, data) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden postMessage listener: validate origin and source
Currently accepts any origin with only a data.source check. Validate event.origin against an allowlist and ensure event.source === window.parent.
- window.addEventListener('message', (event) => {
- const { data } = event
- if (data.source === 'kradle-frontend') {
+ const allowedParentOrigins = new Set<string>() // TODO: populate from config
+ window.addEventListener('message', (event) => {
+ const { data, origin, source } = event
+ if (source === window.parent && data?.source === 'kradle-frontend' && (allowedParentOrigins.size === 0 || allowedParentOrigins.has(origin))) {
console.log('[iframe-rpc] [minecraft-web-client] Received message', data)
customEvents.emit(`kradle:${data.action as ReceivableActions}`, data)
}
})📝 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.
| window.addEventListener('message', (event) => { | |
| const { data } = event | |
| if (data.source === 'kradle-frontend') { | |
| console.log('[iframe-rpc] [minecraft-web-client] Received message', data) | |
| customEvents.emit(`kradle:${data.action as ReceivableActions}`, data) | |
| } | |
| }) | |
| // TODO: Populate allowedParentOrigins from your runtime configuration | |
| const allowedParentOrigins = new Set<string>() | |
| window.addEventListener('message', (event) => { | |
| const { data, origin, source } = event | |
| if ( | |
| source === window.parent && | |
| data?.source === 'kradle-frontend' && | |
| (allowedParentOrigins.size === 0 || allowedParentOrigins.has(origin)) | |
| ) { | |
| console.log('[iframe-rpc] [minecraft-web-client] Received message', data) | |
| customEvents.emit(`kradle:${data.action as ReceivableActions}`, data) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/iframe.ts around lines 37 to 43, the postMessage handler currently trusts
any origin and only checks data.source; update it to first verify event.origin
is in a defined allowlist (create a constant array of allowed origins and
compare exactly), ensure event.source === window.parent (reject messages from
other windows/iframes), and also guard that event.data exists and has the
expected shape before emitting; if checks fail, return early and optionally log
a warning.
| // Handle outgoing messages to kradle frontend | ||
| function sendMessageToKradle ( | ||
| payload: Omit<IFrameSendablePayload, 'source'> | ||
| ) { | ||
| if (window !== window.parent) { | ||
| window.parent.postMessage({ | ||
| ...payload, | ||
| source: 'minecraft-web-client' | ||
| }, '*') | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid '*' in postMessage; use the validated parent origin
Store and use the parent’s origin to prevent data exfiltration to arbitrary listeners.
- function sendMessageToKradle (
+ let parentOrigin: string | undefined
+ window.addEventListener('message', (event) => {
+ if (event.source === window.parent && event.data?.source === 'kradle-frontend') {
+ parentOrigin = event.origin
+ }
+ })
+
+ function sendMessageToKradle (
payload: Omit<IFrameSendablePayload, 'source'>
) {
if (window !== window.parent) {
window.parent.postMessage({
...payload,
source: 'minecraft-web-client'
- }, '*')
+ }, parentOrigin ?? '*')
}
}📝 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.
| // Handle outgoing messages to kradle frontend | |
| function sendMessageToKradle ( | |
| payload: Omit<IFrameSendablePayload, 'source'> | |
| ) { | |
| if (window !== window.parent) { | |
| window.parent.postMessage({ | |
| ...payload, | |
| source: 'minecraft-web-client' | |
| }, '*') | |
| } | |
| } | |
| // Handle outgoing messages to kradle frontend | |
| let parentOrigin: string | undefined | |
| window.addEventListener('message', (event) => { | |
| if (event.source === window.parent && event.data?.source === 'kradle-frontend') { | |
| parentOrigin = event.origin | |
| } | |
| }) | |
| function sendMessageToKradle ( | |
| payload: Omit<IFrameSendablePayload, 'source'> | |
| ) { | |
| if (window !== window.parent) { | |
| window.parent.postMessage({ | |
| ...payload, | |
| source: 'minecraft-web-client' | |
| }, parentOrigin ?? '*') | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/iframe.ts around lines 45–55, the postMessage call uses '*' which allows
any origin to receive messages; instead compute and reuse a validated parent
origin (e.g., parse document.referrer with new URL(document.referrer).origin or
obtain a stored parentOrigin passed in on init), validate it against an
allowlist or ensure it is non-empty, and then pass that origin as the second
argument to window.parent.postMessage; if the origin cannot be
determined/validated, do not send the message and log or handle the error.
| // Setup packet monitoring for replay information | ||
| function setupPacketMonitoring () { | ||
| if (!bot || !bot._client) { | ||
| console.log('[packet-monitor] Bot not ready yet, retrying in 1s') | ||
| setTimeout(setupPacketMonitoring, 1000) | ||
| return | ||
| } | ||
|
|
||
| console.log( | ||
| '[packet-monitor] Setting up packet monitoring for replay data' | ||
| ) | ||
|
|
||
| // Monitor boss_bar packets for replay progress and broadcast to parent | ||
| let lastReplayStatus: any = null | ||
| let storedProgress = 0 | ||
| let storedPercentage = 0 | ||
| let storedCurrentTime = '' | ||
| let storedRecordingName = '' | ||
|
|
||
| bot._client.on('boss_bar', (data) => { | ||
| // Extract progress percentage (action 2) | ||
| if (data.health !== undefined) { | ||
| storedProgress = data.health | ||
| storedPercentage = Math.round(data.health * 100) | ||
| } | ||
|
|
||
| // Extract time and recording name from title (action 3) | ||
| if ( | ||
| data.title?.value?.extra?.value?.value | ||
| ) { | ||
| try { | ||
| const extraItems = data.title.value.extra.value.value | ||
| for (const item of extraItems) { | ||
| if (item.text?.value) { | ||
| const text = item.text.value | ||
| if (/\d{2}:\d{2}:\d{2}/.test(text)) { | ||
| storedCurrentTime = text | ||
| } else if (/\d{4}-\d{2}-\d{2}--\d{2}-\d{2}-\d{2}/.test(text)) { | ||
| storedRecordingName = text | ||
| } | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.log('[replay-parse-error]', e.message) | ||
| } | ||
| } | ||
|
|
||
| // Create status object from stored values | ||
| const replayStatus = { | ||
| currentTime: storedCurrentTime, | ||
| progress: storedProgress, | ||
| percentage: storedPercentage, | ||
| recordingName: storedRecordingName, | ||
| } | ||
|
|
||
| // Only send if data has changed and we have minimum required data | ||
| const statusChanged = | ||
| JSON.stringify(replayStatus) !== JSON.stringify(lastReplayStatus) | ||
| if (statusChanged && storedCurrentTime && window !== window.parent) { | ||
| sendMessageToKradle({ | ||
| action: 'replayStatus', | ||
| ...replayStatus, | ||
| }) | ||
|
|
||
| lastReplayStatus = replayStatus | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Detach 'boss_bar' listener on bot end to prevent leaks across reconnects
Listeners persist across sessions; add cleanup on bot 'end'.
- bot._client.on('boss_bar', (data) => {
+ const onBossBar = (data) => {
// Extract progress percentage (action 2)
if (data.health !== undefined) {
storedProgress = data.health
storedPercentage = Math.round(data.health * 100)
}
// ... unchanged ...
- })
+ }
+ bot._client.on('boss_bar', onBossBar)
+ bot.once('end', () => {
+ // @ts-expect-error optional off
+ bot._client.off?.('boss_bar', onBossBar)
+ bot._client.removeListener?.('boss_bar', onBossBar)
+ })📝 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.
| // Setup packet monitoring for replay information | |
| function setupPacketMonitoring () { | |
| if (!bot || !bot._client) { | |
| console.log('[packet-monitor] Bot not ready yet, retrying in 1s') | |
| setTimeout(setupPacketMonitoring, 1000) | |
| return | |
| } | |
| console.log( | |
| '[packet-monitor] Setting up packet monitoring for replay data' | |
| ) | |
| // Monitor boss_bar packets for replay progress and broadcast to parent | |
| let lastReplayStatus: any = null | |
| let storedProgress = 0 | |
| let storedPercentage = 0 | |
| let storedCurrentTime = '' | |
| let storedRecordingName = '' | |
| bot._client.on('boss_bar', (data) => { | |
| // Extract progress percentage (action 2) | |
| if (data.health !== undefined) { | |
| storedProgress = data.health | |
| storedPercentage = Math.round(data.health * 100) | |
| } | |
| // Extract time and recording name from title (action 3) | |
| if ( | |
| data.title?.value?.extra?.value?.value | |
| ) { | |
| try { | |
| const extraItems = data.title.value.extra.value.value | |
| for (const item of extraItems) { | |
| if (item.text?.value) { | |
| const text = item.text.value | |
| if (/\d{2}:\d{2}:\d{2}/.test(text)) { | |
| storedCurrentTime = text | |
| } else if (/\d{4}-\d{2}-\d{2}--\d{2}-\d{2}-\d{2}/.test(text)) { | |
| storedRecordingName = text | |
| } | |
| } | |
| } | |
| } catch (e) { | |
| console.log('[replay-parse-error]', e.message) | |
| } | |
| } | |
| // Create status object from stored values | |
| const replayStatus = { | |
| currentTime: storedCurrentTime, | |
| progress: storedProgress, | |
| percentage: storedPercentage, | |
| recordingName: storedRecordingName, | |
| } | |
| // Only send if data has changed and we have minimum required data | |
| const statusChanged = | |
| JSON.stringify(replayStatus) !== JSON.stringify(lastReplayStatus) | |
| if (statusChanged && storedCurrentTime && window !== window.parent) { | |
| sendMessageToKradle({ | |
| action: 'replayStatus', | |
| ...replayStatus, | |
| }) | |
| lastReplayStatus = replayStatus | |
| } | |
| }) | |
| } | |
| // Monitor boss_bar packets for replay progress and broadcast to parent | |
| let lastReplayStatus: any = null | |
| let storedProgress = 0 | |
| let storedPercentage = 0 | |
| let storedCurrentTime = '' | |
| let storedRecordingName = '' | |
| const onBossBar = (data) => { | |
| // Extract progress percentage (action 2) | |
| if (data.health !== undefined) { | |
| storedProgress = data.health | |
| storedPercentage = Math.round(data.health * 100) | |
| } | |
| // Extract time and recording name from title (action 3) | |
| if ( | |
| data.title?.value?.extra?.value?.value | |
| ) { | |
| try { | |
| const extraItems = data.title.value.extra.value.value | |
| for (const item of extraItems) { | |
| if (item.text?.value) { | |
| const text = item.text.value | |
| if (/\d{2}:\d{2}:\d{2}/.test(text)) { | |
| storedCurrentTime = text | |
| } else if (/\d{4}-\d{2}-\d{2}--\d{2}-\d{2}-\d{2}/.test(text)) { | |
| storedRecordingName = text | |
| } | |
| } | |
| } | |
| } catch (e) { | |
| console.log('[replay-parse-error]', e.message) | |
| } | |
| } | |
| // Create status object from stored values | |
| const replayStatus = { | |
| currentTime: storedCurrentTime, | |
| progress: storedProgress, | |
| percentage: storedPercentage, | |
| recordingName: storedRecordingName, | |
| } | |
| // Only send if data has changed and we have minimum required data | |
| const statusChanged = | |
| JSON.stringify(replayStatus) !== JSON.stringify(lastReplayStatus) | |
| if (statusChanged && storedCurrentTime && window !== window.parent) { | |
| sendMessageToKradle({ | |
| action: 'replayStatus', | |
| ...replayStatus, | |
| }) | |
| lastReplayStatus = replayStatus | |
| } | |
| } | |
| bot._client.on('boss_bar', onBossBar) | |
| bot.once('end', () => { | |
| // @ts-expect-error optional off | |
| bot._client.off?.('boss_bar', onBossBar) | |
| bot._client.removeListener?.('boss_bar', onBossBar) | |
| }) |
🤖 Prompt for AI Agents
In src/iframe.ts around lines 154 to 221, the boss_bar listener is added on
every setup but never removed, causing event listener leaks across reconnects;
fix by assigning the boss_bar callback to a named/const handler, register that
handler with bot._client.on('boss_bar', handler), and then attach a cleanup that
removes it when the bot ends (e.g., bot.once('end' or bot._client.once('end'),
call bot._client.off('boss_bar', handler)). Also ensure any previous handler is
removed before re-registering if setupPacketMonitoring can run multiple times.
|
sorry this was not meant to be for this repo. it was meant for a fork. apologies. |
|
Sorry, can I keep it open for visibility please 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/optionsStorage.ts (2)
141-146: Harden QS parsing to avoid runtime crashes on malformed JSON.Blind
JSON.parsewill throw and blank the app. Fall back to bool/number/string.-const qsOptionsRaw = appQueryParamsArray.setting ?? [] -export const qsOptions = Object.fromEntries(qsOptionsRaw.map(o => { - const [key, value] = o.split(':') - return [key, JSON.parse(value)] -})) +const qsOptionsRaw = appQueryParamsArray.setting ?? [] +const parseQSValue = (v: string) => { + try { return JSON.parse(v) } catch { + if (v === 'true' || v === 'false') return v === 'true' + const n = Number(v) + return Number.isNaN(n) ? v : n + } +} +export const qsOptions = Object.fromEntries(qsOptionsRaw.map(o => { + const idx = o.indexOf(':') + const key = idx === -1 ? o : o.slice(0, idx) + const value = idx === -1 ? 'true' : o.slice(idx + 1) + return [key, parseQSValue(value)] +}))
282-286:useOptionValuecallback signature misuse and missing unsubscribe.
subscribepasses ops, not the setting. Wrap it and return the unsubscribe.-export const useOptionValue = (setting, valueCallback) => { - valueCallback(setting) - subscribe(setting, valueCallback) -} +export const useOptionValue = <T>(setting: T, valueCallback: (v: T) => void) => { + valueCallback(setting) + const unsubscribe = subscribe(setting as any, () => valueCallback(setting)) + return unsubscribe +}
♻️ Duplicate comments (3)
src/iframe.ts (3)
37-43: Harden postMessage listener: validate source window, origin, and action- window.addEventListener('message', (event) => { - const { data } = event - if (data.source === 'kradle-frontend') { + window.addEventListener('message', (event) => { + const { data, origin, source } = event + if (source !== window.parent) return + if (!data || data.source !== 'kradle-frontend') return + if (ALLOWED_PARENT_ORIGINS.size > 0 && !ALLOWED_PARENT_ORIGINS.has(origin)) return + if (!isReceivableAction(data.action)) return + parentOrigin = origin console.log('[iframe-rpc] [minecraft-web-client] Received message', data) - customEvents.emit(`kradle:${data.action as ReceivableActions}`, data) + customEvents.emit(`kradle:${data.action}`, data) } })
45-55: Avoid '*' in postMessage; send only to validated parent originfunction sendMessageToKradle ( payload: Omit<IFrameSendablePayload, 'source'> ) { if (window !== window.parent) { + if (!parentOrigin) { + console.warn('[iframe-rpc] parentOrigin unknown; dropping', payload.action) + return + } window.parent.postMessage({ ...payload, source: 'minecraft-web-client' - }, '*') + }, parentOrigin) } }
169-217: Detach 'boss_bar' listener on bot end to prevent leaks across reconnects- bot._client.on('boss_bar', (data) => { + const onBossBar = (data: any) => { // Extract progress percentage (action 2) if (data.health !== undefined) { storedProgress = data.health storedPercentage = Math.round(data.health * 100) } // Extract time and recording name from title (action 3) if ( data.title?.value?.extra?.value?.value ) { try { const extraItems = data.title.value.extra.value.value for (const item of extraItems) { if (item.text?.value) { const text = item.text.value if (/\d{2}:\d{2}:\d{2}/.test(text)) { storedCurrentTime = text } else if (/\d{4}-\d{2}-\d{2}--\d{2}-\d{2}-\d{2}/.test(text)) { storedRecordingName = text } } } } catch (e) { console.log('[replay-parse-error]', e.message) } } // Create status object from stored values const replayStatus = { currentTime: storedCurrentTime, progress: storedProgress, percentage: storedPercentage, recordingName: storedRecordingName, } // Only send if data has changed and we have minimum required data const statusChanged = JSON.stringify(replayStatus) !== JSON.stringify(lastReplayStatus) if (statusChanged && storedCurrentTime && window !== window.parent) { sendMessageToKradle({ action: 'replayStatus', ...replayStatus, }) lastReplayStatus = replayStatus } - }) + } + bot._client.on('boss_bar', onBossBar) + bot.once('end', () => { + // mineflayer client may expose either off/removeListener + // @ts-expect-error optional off + bot._client.off?.('boss_bar', onBossBar) + bot._client.removeListener?.('boss_bar', onBossBar) + })
🧹 Nitpick comments (7)
src/optionsStorage.ts (5)
185-199: Deep-equal: acceptable, but consider a vetted lib for correctness/perf.Edge cases (cyclic refs, Dates, Sets/Maps) aren’t handled. If you need broader coverage, use
fast-deep-equalordequal.
217-227: Augment Window typings foroptions/settings/debugChangedOptions.Avoid TS errors by declaring globals.
Add a .d.ts (e.g.,
src/types/window.d.ts):import type { AppOptions } from '../optionsStorage' declare global { interface Window { options: AppOptions settings: AppOptions debugChangedOptions: Record<string, unknown> } } export {}
148-152: Comment says “forced by config” but only QS is respected.If config should lock settings, seed
disabledSettingswith those keys too (e.g.,initialAppConfig.lockedSettings).I can add
lockedSettings?: string[]toAppConfigand wire it here.
212-215: QS merge is shallow; nested keys won’t apply as intended.If QS supports dotted paths, implement a safe deep set before the final spread.
Example helper (outside diff):
const setByPath = (obj: any, path: string, value: any) => { const parts = path.split('.') let cur = obj for (let i = 0; i < parts.length - 1; i++) cur = (cur[parts[i]] ??= {}) cur[parts.at(-1)!] = value }Apply it to
qsOptionsbefore creatingoptions.
3-3: Unused import.
omitObjis not used in this module.-import { omitObj } from '@zardoy/utils'src/iframe.ts (2)
142-148: Type safety: ensurestatusDatamatches theconnectionStatuspayload shapeConfirm
statusDataalways includes:status,message,canReconnect(and optionalerrorDetails). If not, narrow or map before spreading to avoid emitting malformed payloads. Would you like a small type helper for this?
219-225: Be consistent in emitter usageYou use
customEventselsewhere but switch towindow.customEventshere. Prefer a single reference to avoid confusion:- if (window?.customEvents) { - window.customEvents.on('mineflayerBotCreated', () => { + if (customEvents) { + customEvents.on('mineflayerBotCreated', () => { console.log('[packet-monitor] Bot created, setting up packet monitoring') setTimeout(setupPacketMonitoring, 1000) // Give bot time to initialize }) }If
customEventsmay be undefined at module load, gate it similarly with optional chaining.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/iframe.ts(1 hunks)src/optionsStorage.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/vars-usage.mdc)
src/**/*.ts: You may use the global variablebotdirectly in any file under thesrc/directory (e.g.,src/mineflayer/playerState.ts).
Insrc/code, you may use the global variableappViewerfromsrc/appViewer.tsdirectly. Do not importappVieweror usewindow.appViewer; use the globalappViewervariable as-is.
Files:
src/iframe.tssrc/optionsStorage.ts
🧬 Code graph analysis (1)
src/iframe.ts (2)
src/optionsStorage.ts (1)
options(210-215)src/sounds/musicSystem.ts (1)
musicSystem(45-45)
🔇 Additional comments (1)
src/optionsStorage.ts (1)
206-209: Avoid hard-reverting saved volume on every load.Unconditionally deleting
volumenegates user preference. Gate by a config flag or remove.Proposed guard:
-// Force default volume on load (remove this line if you want to remember user's volume preference) -delete appStorage.changedSettings.volume +// Optional: force default volume only if requested by config +if ((initialAppConfig as any)?.forceDefaultVolumeOnLoad) { + delete appStorage.changedSettings.volume +}Would you like me to wire
forceDefaultVolumeOnLoadintoAppConfig?
| type ReceivableActions = 'followPlayer' | 'command' | 'reconnect' | 'setAgentSkins' | 'setVolume' | 'setMusic' | ||
|
|
||
| export function setupIframeComms () { | ||
| // Handle incoming messages from kradle frontend |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add allowlists and a type guard before wiring messaging
Define a parent-origin allowlist and an action type guard now; used by both the inbound listener and postMessage.
type ReceivableActions = 'followPlayer' | 'command' | 'reconnect' | 'setAgentSkins' | 'setVolume' | 'setMusic'
export function setupIframeComms () {
+ // TODO: populate via env/config
+ const ALLOWED_PARENT_ORIGINS = new Set<string>([
+ // 'https://kradle.app',
+ // 'http://localhost:5173'
+ ])
+ let parentOrigin: string | undefined
+ const referrerOrigin = (() => {
+ try { return document.referrer ? new URL(document.referrer).origin : undefined } catch { return undefined }
+ })()
+ if (referrerOrigin) parentOrigin = referrerOrigin
+
+ const isReceivableAction = (a: any): a is ReceivableActions =>
+ a === 'followPlayer' || a === 'command' || a === 'reconnect' ||
+ a === 'setAgentSkins' || a === 'setVolume' || a === 'setMusic'📝 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.
| type ReceivableActions = 'followPlayer' | 'command' | 'reconnect' | 'setAgentSkins' | 'setVolume' | 'setMusic' | |
| export function setupIframeComms () { | |
| // Handle incoming messages from kradle frontend | |
| type ReceivableActions = 'followPlayer' | 'command' | 'reconnect' | 'setAgentSkins' | 'setVolume' | 'setMusic' | |
| export function setupIframeComms () { | |
| // TODO: populate via env/config | |
| const ALLOWED_PARENT_ORIGINS = new Set<string>([ | |
| // 'https://kradle.app', | |
| // 'http://localhost:5173' | |
| ]) | |
| let parentOrigin: string | undefined | |
| const referrerOrigin = (() => { | |
| try { | |
| return document.referrer ? new URL(document.referrer).origin : undefined | |
| } catch { | |
| return undefined | |
| } | |
| })() | |
| if (referrerOrigin) parentOrigin = referrerOrigin | |
| const isReceivableAction = (a: any): a is ReceivableActions => | |
| a === 'followPlayer' || a === 'command' || a === 'reconnect' || | |
| a === 'setAgentSkins' || a === 'setVolume' || a === 'setMusic' | |
| // Handle incoming messages from kradle frontend |
| customEvents.on('kradle:command', (data) => { | ||
| const { command } = data | ||
| if (!command) { | ||
| console.error('No command provided') | ||
| return | ||
| } | ||
|
|
||
| const formattedCommand = `/${command.replace(/^\//, '')}` | ||
| console.log('[packet-monitor] Sending command to bot:', formattedCommand) | ||
| bot.chat(formattedCommand) | ||
| }) |
There was a problem hiding this comment.
Guard bot readiness and sanitize command before chat
customEvents.on('kradle:command', (data) => {
- const { command } = data
- if (!command) {
+ const { command } = data ?? {}
+ if (typeof command !== 'string' || !command.trim()) {
console.error('No command provided')
return
}
- const formattedCommand = `/${command.replace(/^\//, '')}`
+ const formattedCommand = `/${command.replace(/^\//, '').slice(0, 256)}`
console.log('[packet-monitor] Sending command to bot:', formattedCommand)
- bot.chat(formattedCommand)
+ try {
+ // global `bot` is allowed in src/
+ if (!bot?.chat) throw new Error('bot not ready')
+ bot.chat(formattedCommand)
+ } catch (err) {
+ console.error('[iframe-rpc] Failed to send command:', err)
+ }
})📝 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.
| customEvents.on('kradle:command', (data) => { | |
| const { command } = data | |
| if (!command) { | |
| console.error('No command provided') | |
| return | |
| } | |
| const formattedCommand = `/${command.replace(/^\//, '')}` | |
| console.log('[packet-monitor] Sending command to bot:', formattedCommand) | |
| bot.chat(formattedCommand) | |
| }) | |
| customEvents.on('kradle:command', (data) => { | |
| const { command } = data ?? {} | |
| if (typeof command !== 'string' || !command.trim()) { | |
| console.error('No command provided') | |
| return | |
| } | |
| const formattedCommand = `/${command.replace(/^\//, '').slice(0, 256)}` | |
| console.log('[packet-monitor] Sending command to bot:', formattedCommand) | |
| try { | |
| // global `bot` is allowed in src/ | |
| if (!bot?.chat) throw new Error('bot not ready') | |
| bot.chat(formattedCommand) | |
| } catch (err) { | |
| console.error('[iframe-rpc] Failed to send command:', err) | |
| } | |
| }) |
| // Handle agent skin data from parent app | ||
| customEvents.on('kradle:setAgentSkins', (data) => { | ||
| console.log('[iframe-rpc] Agent skin data received from parent', data) | ||
| // Store agent skin data globally for use by entities | ||
| if (window.agentSkinMap) { | ||
| window.agentSkinMap.clear() | ||
| } else { | ||
| window.agentSkinMap = new Map() | ||
| } | ||
|
|
||
| if (data.agentSkins) { | ||
| for (const agentSkin of data.agentSkins) { | ||
| if (agentSkin.username && agentSkin.skinUrl) { | ||
| // Primary mapping: username -> skinUrl | ||
| window.agentSkinMap.set(agentSkin.username, agentSkin.skinUrl) | ||
| } | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate and sanitize agent skin payload to avoid bad URLs
customEvents.on('kradle:setAgentSkins', (data) => {
console.log('[iframe-rpc] Agent skin data received from parent', data)
// Store agent skin data globally for use by entities
if (window.agentSkinMap) {
window.agentSkinMap.clear()
} else {
window.agentSkinMap = new Map()
}
- if (data.agentSkins) {
- for (const agentSkin of data.agentSkins) {
- if (agentSkin.username && agentSkin.skinUrl) {
- // Primary mapping: username -> skinUrl
- window.agentSkinMap.set(agentSkin.username, agentSkin.skinUrl)
- }
- }
- }
+ if (!Array.isArray(data?.agentSkins)) {
+ console.warn('[iframe-rpc] Invalid agentSkins payload')
+ return
+ }
+ for (const agentSkin of data.agentSkins) {
+ const username = agentSkin?.username
+ const skinUrl = agentSkin?.skinUrl
+ if (typeof username === 'string' && typeof skinUrl === 'string') {
+ try {
+ const url = new URL(skinUrl, location.origin)
+ if (url.protocol === 'http:' || url.protocol === 'https:') {
+ // username -> skinUrl
+ window.agentSkinMap.set(username, url.toString())
+ }
+ } catch {
+ /* ignore invalid URL */
+ }
+ }
+ }
})📝 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.
| // Handle agent skin data from parent app | |
| customEvents.on('kradle:setAgentSkins', (data) => { | |
| console.log('[iframe-rpc] Agent skin data received from parent', data) | |
| // Store agent skin data globally for use by entities | |
| if (window.agentSkinMap) { | |
| window.agentSkinMap.clear() | |
| } else { | |
| window.agentSkinMap = new Map() | |
| } | |
| if (data.agentSkins) { | |
| for (const agentSkin of data.agentSkins) { | |
| if (agentSkin.username && agentSkin.skinUrl) { | |
| // Primary mapping: username -> skinUrl | |
| window.agentSkinMap.set(agentSkin.username, agentSkin.skinUrl) | |
| } | |
| } | |
| } | |
| }) | |
| // Handle agent skin data from parent app | |
| customEvents.on('kradle:setAgentSkins', (data) => { | |
| console.log('[iframe-rpc] Agent skin data received from parent', data) | |
| // Store agent skin data globally for use by entities | |
| if (window.agentSkinMap) { | |
| window.agentSkinMap.clear() | |
| } else { | |
| window.agentSkinMap = new Map() | |
| } | |
| if (!Array.isArray(data?.agentSkins)) { | |
| console.warn('[iframe-rpc] Invalid agentSkins payload') | |
| return | |
| } | |
| for (const agentSkin of data.agentSkins) { | |
| const username = agentSkin?.username | |
| const skinUrl = agentSkin?.skinUrl | |
| if (typeof username === 'string' && typeof skinUrl === 'string') { | |
| try { | |
| const url = new URL(skinUrl, location.origin) | |
| if (url.protocol === 'http:' || url.protocol === 'https:') { | |
| // username -> skinUrl | |
| window.agentSkinMap.set(username, url.toString()) | |
| } | |
| } catch { | |
| /* ignore invalid URL */ | |
| } | |
| } | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/iframe.ts around lines 97 to 115, the handler stores agent skin URLs
directly from the parent payload without validation or sanitization; update it
to validate each agentSkin entry by ensuring username and skinUrl are strings,
trim them, and construct a URL object inside a try/catch to confirm it is
well-formed; additionally enforce an allowed-schemes whitelist (e.g., only
https: and optionally http:) and optionally check the pathname has a common
image extension (png/jpg/jpeg/webp) or a safe host pattern before calling
window.agentSkinMap.set; if validation fails, skip that entry and log a warning
(don’t throw) so malformed or unsafe URLs are not stored.
| import { appStorage } from './react/appStorageProvider' | ||
|
|
||
| const isDev = process.env.NODE_ENV === 'development' | ||
| const initialAppConfig = process.env?.INLINED_APP_CONFIG as AppConfig ?? {} |
There was a problem hiding this comment.
Fix unsafe INLINED_APP_CONFIG parsing and undefined spread (can crash on startup).
initialAppConfig.defaultSettings may be undefined (or a JSON string), and { ...undefined } throws. Parse safely and guard when spreading.
-const initialAppConfig = process.env?.INLINED_APP_CONFIG as AppConfig ?? {}
+const initialAppConfig: AppConfig = (() => {
+ const raw = (process.env as any)?.INLINED_APP_CONFIG
+ if (!raw) return {} as AppConfig
+ try {
+ return (typeof raw === 'string' ? JSON.parse(raw) : raw) as AppConfig
+ } catch {
+ console.warn('Invalid INLINED_APP_CONFIG, falling back to empty config')
+ return {} as AppConfig
+ }
+})() export const options: AppOptions = proxy({
...defaultOptions,
- ...initialAppConfig.defaultSettings,
- ...migrateOptions(appStorage.changedSettings),
+ ...((initialAppConfig?.defaultSettings ?? {}) as Partial<AppOptions>),
+ ...migrateOptions(appStorage.changedSettings ?? {}),
...qsOptions
})Also applies to: 212-215
🤖 Prompt for AI Agents
In src/optionsStorage.ts around line 9 (and similarly lines 212-215), the
current initialization reads INLINED_APP_CONFIG unsafely and may spread
undefined (causing a crash); instead, safely parse the environment variable with
a try/catch fallback to an empty object, ensure the parsed value is an object,
and when merging/defaulting use nullish checks before spreading (e.g., treat
defaultSettings as an object or fallback to {} before doing { ...defaultSettings
}). Ensure the code never spreads undefined by validating types and using
defaults like {} or Object.assign when merging.
| // Track which settings are disabled (controlled by QS or forced by config) | ||
| export const disabledSettings = proxy({ | ||
| value: new Set<string>(Object.keys(qsOptions)) | ||
| }) | ||
|
|
There was a problem hiding this comment.
QS-controlled settings can still be overwritten for nested keys.
disabledSettings holds raw keys from QS, but the subscriber blocks by path[0]. If QS passes a dotted path (e.g., rendererOptions.three._renderByChunks), edits to nested props aren’t blocked. Track both the full path and its top-level segment.
-export const disabledSettings = proxy({
- value: new Set<string>(Object.keys(qsOptions))
-})
+export const disabledSettings = proxy({
+ value: new Set<string>([
+ ...Object.keys(qsOptions),
+ ...Object.keys(qsOptions).map(k => k.split('.')[0]),
+ ]),
+})- const key = path[0] as string
- if (disabledSettings.value.has(key)) continue
+ const key = path[0] as string
+ const fullPath = (path as Array<string | number>).join('.')
+ if (disabledSettings.value.has(key) || disabledSettings.value.has(fullPath)) continueAlso applies to: 236-239
🤖 Prompt for AI Agents
In src/optionsStorage.ts around lines 148-152 (and likewise update the similar
logic at 236-239), disabledSettings currently stores only raw QS keys which
allows edits to nested properties when QS provided a dotted path; update
disabledSettings to record both the full QS path and its top-level segment
(i.e., add both the full dotted key and path.split('.')[0]) when initializing or
adding entries so the subscriber that blocks by path[0] will block nested edits
as well; ensure additions and removals keep both representations in sync.
| const migrateOptionsLocalStorage = () => { | ||
| if (Object.keys(appStorage.options).length) { | ||
| for (const key of Object.keys(appStorage.options)) { | ||
| if (!(key in defaultOptions)) continue // drop unknown options | ||
| const defaultValue = defaultOptions[key] | ||
| if (JSON.stringify(defaultValue) !== JSON.stringify(appStorage.options[key])) { | ||
| appStorage.changedSettings[key] = appStorage.options[key] | ||
| } | ||
| } | ||
| appStorage.options = {} | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make migration resilient and avoid stringify-based equality.
Guard undefined appStorage.options and reuse isDeepEqual for structural compare.
-const migrateOptionsLocalStorage = () => {
- if (Object.keys(appStorage.options).length) {
- for (const key of Object.keys(appStorage.options)) {
+const migrateOptionsLocalStorage = () => {
+ const legacy = appStorage.options ?? {}
+ if (Object.keys(legacy).length) {
+ for (const key of Object.keys(legacy)) {
if (!(key in defaultOptions)) continue // drop unknown options
- const defaultValue = defaultOptions[key]
- if (JSON.stringify(defaultValue) !== JSON.stringify(appStorage.options[key])) {
- appStorage.changedSettings[key] = appStorage.options[key]
+ const defaultValue = (defaultOptions as any)[key]
+ const legacyValue = (legacy as any)[key]
+ if (!isDeepEqual(defaultValue, legacyValue)) {
+ appStorage.changedSettings[key] = legacyValue
}
}
appStorage.options = {}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const migrateOptionsLocalStorage = () => { | |
| if (Object.keys(appStorage.options).length) { | |
| for (const key of Object.keys(appStorage.options)) { | |
| if (!(key in defaultOptions)) continue // drop unknown options | |
| const defaultValue = defaultOptions[key] | |
| if (JSON.stringify(defaultValue) !== JSON.stringify(appStorage.options[key])) { | |
| appStorage.changedSettings[key] = appStorage.options[key] | |
| } | |
| } | |
| appStorage.options = {} | |
| } | |
| } | |
| const migrateOptionsLocalStorage = () => { | |
| const legacy = appStorage.options ?? {} | |
| if (Object.keys(legacy).length) { | |
| for (const key of Object.keys(legacy)) { | |
| if (!(key in defaultOptions)) continue // drop unknown options | |
| const defaultValue = (defaultOptions as any)[key] | |
| const legacyValue = (legacy as any)[key] | |
| if (!isDeepEqual(defaultValue, legacyValue)) { | |
| appStorage.changedSettings[key] = legacyValue | |
| } | |
| } | |
| appStorage.options = {} | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/optionsStorage.ts around lines 170 to 181, the migration assumes
appStorage.options exists and uses JSON.stringify to compare values; update it
to first guard that appStorage.options is a non-null object (return early if
not), then iterate safely and for each key only consider keys present in
defaultOptions, and replace the JSON.stringify equality check with the shared
isDeepEqual function to compare defaultValue and appStorage.options[key]; also
ensure appStorage.changedSettings is initialized before assigning to it and
preserve the existing clearing of appStorage.options when done.
| subscribe(options, (ops) => { | ||
| for (const op of ops) { | ||
| const [type, path, value] = op | ||
| // let patch | ||
| // let accessor = options | ||
| // for (const part of path) { | ||
| // } | ||
| const key = path[0] as string | ||
| if (disabledSettings.value.has(key)) continue | ||
| appStorage.changedSettings[key] = options[key] | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t persist defaults in changedSettings; clean up on reset.
Currently every change is written; even when equal to defaults, keys linger. Delete keys when value deep-equals default to keep storage minimal and make resetOptions effective.
- appStorage.changedSettings[key] = options[key]
+ const v = options[key]
+ if (isDeepEqual(v, (defaultOptions as any)[key])) {
+ delete appStorage.changedSettings[key]
+ } else {
+ appStorage.changedSettings[key] = v
+ }Optionally, also clear on reset:
export const resetOptions = () => {
Object.assign(options, defaultOptions)
+ appStorage.changedSettings = {}
}📝 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.
| subscribe(options, (ops) => { | |
| for (const op of ops) { | |
| const [type, path, value] = op | |
| // let patch | |
| // let accessor = options | |
| // for (const part of path) { | |
| // } | |
| const key = path[0] as string | |
| if (disabledSettings.value.has(key)) continue | |
| appStorage.changedSettings[key] = options[key] | |
| } | |
| }) | |
| subscribe(options, (ops) => { | |
| for (const op of ops) { | |
| const [type, path, value] = op | |
| // let patch | |
| // let accessor = options | |
| // for (const part of path) { | |
| // } | |
| const key = path[0] as string | |
| if (disabledSettings.value.has(key)) continue | |
| const v = options[key] | |
| if (isDeepEqual(v, (defaultOptions as any)[key])) { | |
| delete appStorage.changedSettings[key] | |
| } else { | |
| appStorage.changedSettings[key] = v | |
| } | |
| } | |
| }) | |
| export const resetOptions = () => { | |
| Object.assign(options, defaultOptions) | |
| appStorage.changedSettings = {} | |
| } |
|
@coderabbitai ignore |
✅ Actions performedReviews paused. |
5 participants
Not meant to be merged
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores