Move minecraft protocol into worker! #319
Conversation
|
/deploy |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-l8h0tc7ws-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-fhqbc3qqq-zaro.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
src/protocolWorker/protocolWorkerGlobals.ts (1)
1-1: Consider removing or documenting commented codeThis commented code doesn't seem to serve a purpose. If it's important for understanding the module, please add an explanatory comment; otherwise, consider removing it.
src/protocolWorker/protocolWorker.ts (4)
14-15: Address the TODO.A TODO comment mentions using another strategy by sending all events. If this is still planned, consider creating an issue or backlog item to track it, ensuring it doesn’t get overlooked.
104-121: Avoid potential concurrency pitfalls when modifying globals.The
registeredChannelsarray is globally tracked, and updates happen in thecall(...)method. While workers run single-threaded, you may wish to encapsulate channel management within a separate class or function to avoid accidental side effects and maintain clearer state management.
123-134: Consider a standard ping approach.
pingProxymanually sends a string message and listens for a “pong” event. Ensure that incoming messages of the same format do not cause collisions or security issues. Alternatively, a standard keepalive approach may reduce the complexity of custom ping/pong logic.
298-299: Address important TODO regarding socket close events.A comment notes that the socket’s close event is never triggered. Investigate whether this stems from the underlying proxy or the environment. This may lead to resource leaks or unexpected behavior.
src/index.ts (2)
124-127: Refactor away fromreduce().The pipeline warns against
Array#reduce()at line 124 (unicorn/no-array-reduce). Consider using a simple loop,forEach, orObject.fromEntriesto constructserverSettingsQs:-const serverSettingsQs = serverSettingsQsRaw.map(x => x.split(':')).reduce<Record<string, string>>((acc, [key, value]) => { - acc[key] = JSON.parse(value) - return acc -}, {}) +const serverSettingsQsEntries = serverSettingsQsRaw.map(x => x.split(':')).map(([k, v]) => [k, JSON.parse(v)]) +const serverSettingsQs = Object.fromEntries(serverSettingsQsEntries)🧰 Tools
🪛 GitHub Actions: CI
[warning] 124-124:
Array#reduce()is not allowed unicorn/no-array-reduce
491-493: Remove the empty block or add a comment.The pipeline flags the empty
else {}statement here (no-empty). Consider removing it entirely or documenting why it is intentionally empty.} else { -} +// else condition intentionally omitted for future extension ... }🧰 Tools
🪛 GitHub Check: build-and-deploy
[failure] 491-491:
Empty block statement🪛 ESLint
[error] 491-493: Empty block statement.
(no-empty)
🪛 GitHub Actions: CI
[error] 491-491: Empty block statement no-empty
renderer/viewer/lib/workerProxy.ts (1)
1-1: Clarify the return type instead of usingvoidin a union.
The usage ofvoid | Promise<any>can be confusing and is flagged by static analysis. Consider using something likePromise<void> | Promise<any>or returningundefinedin synchronous cases to avoid ambiguity.-export function createWorkerProxy<T extends Record<string, (...args: any[]) => void | Promise<any>>> (handlers: T, channel?: MessagePort): { __workerProxy: T } { +export function createWorkerProxy<T extends Record<string, (...args: any[]) => Promise<any> | undefined>> (handlers: T, channel?: MessagePort): { __workerProxy: T } {🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/protocolWorker/protocolMain.ts (1)
52-61: Terminate the Worker explicitly when done.
You have logic to terminate the worker when the bot ends or ifbotis not available, but there’s no fallback for unexpected errors or if the client is reinitialized multiple times. Consider adding robust cleanup or referencing the Worker in a higher-level lifecycle.src/protocolWorker/microsoftAuthflow.ts (2)
20-20: Consider returning strongly-typed data structures.
The function signature returns an object with a shallow type but sets everything toany. You may benefit from specifying structured interfaces to make the code more maintainable.
115-125: Ensure the message event listener is cleaned up.
This function attaches a 'message' event listener on the worker but doesn’t remove it upon completing the auth flow. If called multiple times, it may cause memory leaks or unexpected behavior.worker.addEventListener('message', function handler({ data }) { if (data.type === 'authFlow') { authData.setConnectingVersion(data.version) resolve() + worker.removeEventListener('message', handler) } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.eslintrc.json(1 hunks)renderer/viewer/lib/simpleUtils.ts(1 hunks)renderer/viewer/lib/workerProxy.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(1 hunks)src/index.ts(9 hunks)src/mineflayer/mc-protocol.ts(1 hunks)src/mineflayer/plugins/ping.ts(1 hunks)src/optionsStorage.ts(1 hunks)src/protocolWorker/microsoftAuthflow.ts(6 hunks)src/protocolWorker/protocolMain.ts(1 hunks)src/protocolWorker/protocolWorker.ts(1 hunks)src/protocolWorker/protocolWorkerGlobals.ts(1 hunks)src/react/AppStatusProvider.tsx(1 hunks)src/shims/minecraftData.ts(1 hunks)src/utils.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/protocolWorker/protocolWorker.ts (3)
renderer/viewer/lib/workerProxy.ts (1)
createWorkerProxy(1-19)src/mineflayer/websocket-core.ts (1)
getWebsocketStream(17-54)src/mineflayer/minecraft-protocol-extra.ts (1)
validatePacket(57-78)
renderer/viewer/lib/simpleUtils.ts (1)
renderer/viewer/lib/worldrendererCommon.ts (1)
version(155-157)
src/index.ts (2)
src/parseServerAddress.ts (1)
parseServerAddress(3-46)src/protocolWorker/protocolMain.ts (1)
getProtocolClientGetter(32-211)
src/protocolWorker/protocolMain.ts (7)
src/protocolWorker/protocolWorker.ts (1)
PROXY_WORKER_TYPE(36-135)src/connect.ts (1)
ConnectOptions(13-42)src/parseServerAddress.ts (1)
ParsedServerAddress(48-54)src/protocolWorker/microsoftAuthflow.ts (2)
getAuthData(20-113)authFlowMainThread(115-164)src/appStatus.ts (1)
setLoadingScreenStatus(5-35)renderer/viewer/lib/workerProxy.ts (1)
useWorkerProxy(32-76)src/appParams.ts (1)
appQueryParams(77-86)
src/protocolWorker/microsoftAuthflow.ts (5)
src/connect.ts (1)
ConnectOptions(13-42)src/react/SignInMessageProvider.tsx (1)
signInMessageState(6-12)src/react/serversStorage.ts (1)
updateAuthenticatedAccountData(82-86)src/react/NotificationProvider.tsx (1)
showNotification(22-37)src/appStatus.ts (1)
setLoadingScreenStatus(5-35)
🪛 GitHub Actions: CI
src/index.ts
[warning] 124-124: Array#reduce() is not allowed unicorn/no-array-reduce
[error] 491-491: Empty block statement no-empty
🪛 Biome (1.9.4)
renderer/viewer/lib/workerProxy.ts
[error] 1-1: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 63-63: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (16)
renderer/viewer/lib/simpleUtils.ts (1)
36-41: Good implementation of the relocatedtoMajorVersionutility.The function provides a clean implementation for extracting major version information and the comment clearly indicates its limitation regarding snapshot versions.
src/optionsStorage.ts (1)
69-69: Good addition of the protocol worker configuration option.Adding
protocolWorkerOptimisation: trueas a default setting aligns with the PR objective of enhancing performance through worker-based architecture for Minecraft protocol tasks. Setting it totrueby default ensures users immediately benefit from the 10% performance improvement on high-load servers..eslintrc.json (1)
202-203: LGTM: ESLint rule addition for protocol worker compatibility.Disabling the
unicorn/relative-url-stylerule is appropriate for this codebase, especially when working with protocol-related code that may need flexibility in URL formatting.src/react/AppStatusProvider.tsx (1)
7-7: Correctly updated import path to reflect new module structure.The import path update for
getProxyDetailsaligns with the PR's architecture changes, moving Microsoft authentication flow logic to the new protocol worker module.src/protocolWorker/protocolWorkerGlobals.ts (1)
3-6: Good setup for Worker environmentAppropriate polyfills for the web worker environment. The ESLint disable is properly explained, and the TypeScript type assertion is necessary to handle the Node.js compatibility expectations in a browser environment.
src/shims/minecraftData.ts (1)
2-2: Import path update looks goodThe import path for
toMajorVersionhas been properly updated to come from the new location inrenderer/viewer/lib/simpleUtils. This aligns with the PR's objective of reorganizing utility functions.src/mineflayer/plugins/ping.ts (2)
2-2: Good addition of worker protocol importThe import of
getProtocolWorkerChannelis correctly added to support the new worker-based architecture for handling Minecraft protocol tasks.
8-11: Well-implemented worker-based ping fallbackThis code correctly implements a fallback for ping functionality when the WebSocket connection isn't available, leveraging the new worker-based architecture. The conditional check is appropriate, and the null coalescing operator handles the case when the protocol worker channel isn't available.
renderer/viewer/lib/worldrendererCommon.ts (2)
17-17: Consolidated imports correctlyThe import statement has been correctly updated to import both
chunkPosandtoMajorVersionfrom the same module, following good practices for import organization.
525-525: Successfully using the relocated utility functionThe
toMajorVersionfunction is correctly used here after its relocation to thesimpleUtilsmodule, ensuring consistent version handling throughout the application.src/mineflayer/mc-protocol.ts (1)
10-12: Reintroduce or confirm removal of packet validation logic.Previously, packet validation calls were removed in favor of simply updating
lastPacketTime. If packet validation was deliberately dropped, confirm that security and stability requirements are still met. Otherwise, consider re-adding a lightweight validation step or verifying that the trust boundary for packets is acceptable.src/protocolWorker/protocolWorker.ts (3)
1-8: Confirm Node modules availability in a Worker environment.This file imports
netfrom Node, which may not be natively available in web workers. Ensure that the bundling environment supports or mocks such modules, or consider limiting import usage to avoid runtime issues in browsers.
137-173: Evaluate user prompt flow for Microsoft authentication.This section offloads and awaits external messages to complete the MS authentication flow. Consider error handling for timeouts in case the main thread never sends
'authflowResult', preventing a potential indefinite wait.
44-103: 🛠️ Refactor suggestionCheck for lost local state with
skipWriteLog.When
data.nameis'write',skipWriteLogis set totrueand never reset tofalse. This one-time override may unintentionally disable logging for all subsequent writes.if (data.name === 'write') { - skipWriteLog = true + skipWriteLog = true + setTimeout(() => { + skipWriteLog = false + }, 0)Likely an incorrect or invalid review comment.
src/protocolWorker/protocolMain.ts (1)
78-152:❓ Verification inconclusive
Validate the usage of global window properties.
This block relies on multiple global properties likewindow.stopPacketsProcessing,window.skipPackets,window.packetTimings, etc. If they are not guaranteed to exist or be valid across different environments, it can lead to runtime errors. Ensure they are declared and maintained carefully or encapsulate them in a local state management structure.
🏁 Script executed:
#!/bin/bash # Searching for definitions of window.stopPacketsProcessing, window.skipPackets, window.packetTimings rg -A 5 'window\.(stopPacketsProcessing|skipPackets|packetTimings)'Length of output: 3437
Validate and encapsulate global window properties usage
The logic in
src/protocolWorker/protocolMain.ts(and related usage inprotocolWorker.ts) appears to rely on several global window properties which are lazily assigned (e.g.packetTimings). While the current checks (e.g. ensuringpacketTimingsexists before assignment and safe guarding array accesses forstopPacketsProcessingandskipPackets) prevent immediate runtime errors in a browser environment, please review the following points:
- Verify that all global properties (such as
stopPacketsProcessing,skipPackets,packetTimings,lastStatsUpdate, andpacketsThreadBlocking) are intentionally kept on the globalwindowobject. This is especially crucial if the code might be executed in non-browser contexts.- Consider encapsulating these properties within a dedicated state management module or object. This can improve maintainability and reduce risks associated with implicit global state reliance.
- Confirm that any external code or initialization logic guarantees that these properties are set as expected across all environments.
Please review these suggestions to ensure robust initialization and avoid any unintended side effects.
src/protocolWorker/microsoftAuthflow.ts (1)
116-146: Secure usage of tokens.
Ensure that this code never logs or accidentally discloses sensitive tokens or credentials (particularly in production). Review data flows carefully to prevent PII leakage.
|
/deploy |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rsbuild.config.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (1)
rsbuild.config.ts (1)
228-229: Good sequence adjustment for service worker generation.Adding the
patchWorkerImport()call beforegenerateSW()ensures all worker scripts have their import paths properly patched before the service worker is generated, which is essential for the protocol worker to function correctly.
|
Deployed to Vercel Preview: https://prismarine-doyllmug7-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/protocolWorker/protocol.worker.ts (1)
281-321: 🛠️ Refactor suggestionGlobal state and
net.Socketprototype modifications
wrapClientSocketupdatesnet.Socket.prototype.handleStringMessageand regards a globallastKnownKickReason. If multiple Worker instances or clients run concurrently, these shared modifications can produce race conditions.src/protocolWorker/protocolMain.ts (1)
212-212: 💡 Verification agent❓ Verification inconclusive
Address build pipeline error
CI logs indicate "NotFound('worker-rspack-loader')". Confirm thatworker-rspack-loaderis installed in dependencies and configured in your build.
🏁 Script executed:
#!/bin/bash # Checking for references to worker-rspack-loader in package.json rg 'worker-rspack-loader' package.jsonLength of output: 38
Action Required: Missing Dependency 'worker-rspack-loader'
- The search in
package.jsondid not reveal any references toworker-rspack-loader, which likely explains the CI error ("NotFound('worker-rspack-loader')").- Please confirm whether this loader is required:
- If it is needed, add it to your dependencies (e.g., via
npm install worker-rspack-loaderor the appropriate package manager) and verify that your build (rspack) configuration properly references it.- If it isn’t needed, remove or update any build configuration or code references to it in
src/protocolWorker/protocolMain.ts(line 212) and related files.
♻️ Duplicate comments (1)
rsbuild.config.ts (1)
264-277: 🛠️ Refactor suggestionThe worker import path patching has potential edge cases.
The function effectively replaces static import paths with dynamic ones based on the current URL, which is good for deployment flexibility. However, there are some concerns:
- The regex replacement is brittle and assumes a specific format for import paths
- The check for multiple
importScriptscalls may not work as expected -match?.lengthcounts capture groups, not matches- No directory existence check before trying to read from it
const patchWorkerImport = () => { + if (!fs.existsSync('./dist/static/js/async')) { + console.warn('Directory ./dist/static/js/async does not exist, skipping worker import patching') + return + } const workerFiles = fs.readdirSync('./dist/static/js/async').filter(x => x.endsWith('.js')) let patched = false for (const file of workerFiles) { const filePath = `./dist/static/js/async/${file}` const content = fs.readFileSync(filePath, 'utf8') - const match = content.match(/importScripts\([^)]+\)/) - if (match?.length > 1) throw new Error('Multiple importScripts found in ' + filePath) + const matches = content.match(/importScripts\([^)]+\)/g) || [] + if (matches.length > 1) throw new Error('Multiple importScripts found in ' + filePath) - const newContent = content.replace(/importScripts\(\w+\.\w+/, "'/'+location.pathname.split('/').slice(0, -4).join('/')") + const newContent = content.replace(/importScripts\((['"])([^'"]+)(['"])\)/, + "importScripts($1" + "/'+location.pathname.split('/').slice(0, -4).join('/')+'/$3)") fs.writeFileSync(filePath, newContent, 'utf8') - patched = true + patched = matches.length > 0 } if (!patched) throw new Error('No importScripts found in any worker files') }
🧹 Nitpick comments (10)
renderer/rsbuildSharedConfig.ts (1)
64-72: Good implementation for worker handling in single file builds.This change correctly sets up the worker-rspack-loader to handle
.worker.jsand.worker.tsfiles when building in single file mode. Theinline: "no-fallback"option ensures these workers are properly bundled inline, which aligns with the PR objective of moving Minecraft protocol handling to a separate worker thread.Consider adding a brief comment explaining why this specific loader configuration was chosen and what "no-fallback" implies for browser compatibility, to help future maintainers understand the design decisions.
src/protocolWorker/protocol.worker.ts (3)
14-18: Optional: Consider a uniform approach for event redirection
There's a TODO comment to eventually send "all events" rather than enumerating them. One approach is to automatically proxy every emitted event from the Minecraft protocol instead of a hardcoded list.
78-85: Heads-up aboutskipWriteLogusage
Overriding theclient.writemethod for packet logging, gated behindskipWriteLog, may obscure important debugging data ifskipWriteLogremains true longer than intended. Consider resetting it automatically or offering a manual toggle to avoid losing valuable logs.
175-223: Add handling for invalid PEM strings
Currently, ifrestoreDataencounters an invalid or corrupted PEM, it won’t throw. Consider a try/catch aroundimportKeyto handle failures gracefully and notify the main thread or user of invalid keys.src/index.ts (3)
180-181: Validate parsed server address
IfconnectOptions.serveris malformed or empty,parseServerAddressmight fail. Add checks to handle invalid results to prevent runtime crashes.
194-194: Caution with detailed logs
PrintingserverParsed.serverIpFullis helpful for debugging, but consider omitting sensitive or internal addresses in production logs for privacy or security compliance.
522-522: Remove commented code
// lastPacket = fullBuffer.toString()is commented out. If no longer needed, removing it would improve clarity and maintainability.src/protocolWorker/protocolMain.ts (3)
19-30: Consider built-in clone or structuredClone
copyPrimitiveValuesselectively clones some properties. If a full deep clone is needed, usingstructuredCloneor a library might reduce boilerplate and handle more data types.
52-62: Ensure coordinated Worker teardown
Terminating the worker via asetTimeoutcallback tied tobot.on('end')or the absence ofbotcan become tricky if multiple bots are created. Evaluate a more robust teardown to avoid worker leaks.
162-179: Performance overhead of JSON serialization
Forwarding method calls throughJSON.stringify/parseis valid, but repeated large data calls can degrade performance. Consider more direct message passing or Transferable objects if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
renderer/rsbuildSharedConfig.ts(1 hunks)rsbuild.config.ts(3 hunks)src/index.ts(9 hunks)src/protocolWorker/protocol.worker.ts(1 hunks)src/protocolWorker/protocolMain.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/protocolWorker/protocol.worker.ts (3)
renderer/viewer/lib/workerProxy.ts (1)
createWorkerProxy(1-19)src/mineflayer/websocket-core.ts (1)
getWebsocketStream(17-54)src/mineflayer/minecraft-protocol-extra.ts (1)
validatePacket(57-78)
🪛 GitHub Actions: CI
rsbuild.config.ts
[error] 207-207: ENOENT: no such file or directory, scandir './dist/single'
src/protocolWorker/protocolMain.ts
[error] 1-1: Build error: Failed to resolve loader: worker-rspack-loader in /home/runner/work/minecraft-web-client/minecraft-web-client, error: NotFound("worker-rspack-loader")
🔇 Additional comments (7)
src/protocolWorker/protocol.worker.ts (1)
137-173:❓ Verification inconclusive
Confirm compatibility of Web Crypto in worker
TheprotocolMicrosoftAuth.authenticateflow andrestoreDatarely onwindow.crypto. Double-check that your Worker environment supports these Web Crypto APIs and that sensitive key material isn’t improperly exposed.
🏁 Script executed:
#!/bin/bash # Checking references to 'crypto' usage in worker code rg -A 3 'window[.]crypto[.]'Length of output: 621
Ensure Worker Compatibility for Web Crypto APIs
The authentication flow (via
protocolMicrosoftAuth.authenticateandrestoreData) depends on Web Crypto. We found that the worker code insrc/protocolWorker/protocol.worker.tsinvokes Web Crypto usingwindow.crypto.subtlefor key import/export. In a traditional Web Worker, however, the global object is typicallyself(orglobalThis), notwindow. Please verify that your target Worker environment provides the expected Web Crypto APIs under the given reference. If not, consider updating the code to useself.cryptoorglobalThis.cryptoto avoid potential runtime issues and ensure that sensitive key material remains secure.src/index.ts (4)
76-76: Smooth usage of reactive updates
Importingsubscribefrom 'valtio' fits well with the rest of the reactive state management. This looks good.
98-98: Centralizing protocol logic
ImportinggetProtocolClientGetteris a neat way to offload protocol tasks to the Worker. This promotes a cleaner architecture by separating connection details from the main thread.
189-190: Check proxy address validity
Similar to server parsing, ensureconnectOptions.proxyis valid. If it's malformed or not set,proxyParsed.hostorproxyParsed.portmay be undefined, causing connection errors.
452-457: Clarify conditional returns inget client()
When certain conditions are met, the getter returnsundefinedinstead of a protocol client. Potentially document or handle the scenario to avoid confusion in code expecting a client instance.src/protocolWorker/protocolMain.ts (2)
13-14: Strong typing for protocol worker
DefiningprotocolWorkerChannelwith a type referencingPROXY_WORKER_TYPEis a good way to maintain type safety when communicating via messages.
193-197: Conditional authentication flow
InvokingauthFlowMainThreadonly ifauthDatais present keeps the worker flexible for both online and offline scenarios. Good approach for modular and optional Microsoft auth.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
rsbuild.config.ts (1)
266-282: 🛠️ Refactor suggestionThe worker import path patching implementation has improved but still has potential issues
The implementation addresses some of the concerns from previous reviews but still has potential issues:
- The regex replacement logic in line 274-275 is still brittle and may not handle all import path formats.
- The function doesn't handle the case where there are no worker files at all.
- The patched flag logic is improved but the error message doesn't provide enough context.
Consider implementing a more robust solution:
const patchWorkerImport = () => { if (!fs.existsSync('./dist/static/js/async')) { console.warn('No async directory found, skipping worker import patching') return } const workerFiles = fs.readdirSync('./dist/static/js/async').filter(x => x.endsWith('.js')) if (workerFiles.length === 0) { console.warn('No worker files found, skipping worker import patching') return } let patched = false for (const file of workerFiles) { const filePath = `./dist/static/js/async/${file}` const content = fs.readFileSync(filePath, 'utf8') const matches = content.match(/importScripts\([^)]+\)/g) || [] if (matches.length > 1) throw new Error('Multiple importScripts found in ' + filePath) - const newContent = content.replace(/importScripts\(\w+\.\w+/, - "importScripts('/'+location.pathname.split('/').slice(0, -4).join('/')") + const newContent = content.replace(/importScripts\((['"])([^'"]+)(['"])\)/, + "importScripts($1" + "/'+location.pathname.split('/').slice(0, -4).join('/')+'/$3)") if (newContent !== content) { fs.writeFileSync(filePath, newContent, 'utf8') patched = true } } if (!patched) throw new Error('No importScripts patched in any worker files. This might indicate a change in the worker code structure.') }Additionally, consider adding debug logging to help with troubleshooting:
console.log(`Patching ${workerFiles.length} worker files in ./dist/static/js/async`)
🧹 Nitpick comments (1)
rsbuild.config.ts (1)
212-217: Add error handling for empty async directory checkGood addition of directory existence check before attempting to read it. This prevents build failures when the directory doesn't exist and aligns with a previous review comment.
However, the current implementation could be more robust with better error messages.
Consider providing more context in the error message to help developers understand the issue:
if (fs.existsSync('./dist/static/js/async')) { const asyncFiles = fs.readdirSync('./dist/static/js/async') if (asyncFiles.length > 0) { - throw new Error('dist/static/js/async must be empty. Ensure workers are imported & built correctly.') + throw new Error('dist/static/js/async must be empty for single file builds. Found files: ' + asyncFiles.join(', ')) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rsbuild.config.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
rsbuild.config.ts
[error] 207-207: Failed to build. ENOENT: no such file or directory, scandir './dist/single'
🔇 Additional comments (1)
rsbuild.config.ts (1)
234-235: Worker import patching added before service worker generationGood placement of the
patchWorkerImport()function call before the service worker generation to ensure worker imports are correctly patched.
|
/protocol |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-r0lk6nejb-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.ts (1)
414-414: Explicit type added for password variable.Adding explicit
string | nulltyping to the password variable improves type safety and code clarity.
🧹 Nitpick comments (1)
src/index.ts (1)
467-467: Remove commented out authentication code.The authentication code is commented out since it's now handled in the protocol worker. It's best practice to remove commented code rather than leaving it in the codebase.
- // auth: connectOptions.authenticatedAccount ? : undefined,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/index.ts(9 hunks)src/optionsStorage.ts(1 hunks)src/utils.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/optionsStorage.ts
🔇 Additional comments (8)
src/index.ts (8)
76-76: Added Valtio subscription support.The new import for
subscribefrom Valtio is properly integrated and will be used later for chunk loading progress tracking.
98-98: Successfully integrated protocol worker architecture.Adding the import for
getProtocolClientGetteraligns with the PR's primary objective of moving protocol handling to a worker thread for improved performance.
181-182: Variable naming improved for clarity.Renaming from
parsedServertoserverParsedcreates more consistent naming patterns throughout the codebase, improving readability.
190-191: Variable naming improved for clarity.Similar to the server variable renaming, changing from
parsedProxytoproxyParsedcreates more consistent naming patterns throughout the codebase.
195-195: Simplified server logging.Using
serverParsed.serverIpFullfor logging consolidates the host and port information into a single, pre-formatted string, which improves code readability.
439-440: Protocol logic successfully moved to worker thread.The complex client creation logic has been refactored to use the new
getProtocolClientGetterfunction, which offloads protocol handling to a dedicated worker thread. This should provide the expected 10% performance improvement on high-load servers mentioned in the PR objectives.
461-466: Client getter logic provides proper fallback paths.The updated client getter supports multiple scenarios: direct streams, singleplayer, p2p, replays, viewer connections, and worker optimizations. The flow control looks correct, providing the worker-based client when appropriate or falling back to undefined for special connection types.
535-536:❓ Verification inconclusive
Validate removal of packet logging.
The packet logging has been commented out as part of the protocol worker refactoring. This appears to be intentional since the protocol is now handled in the worker thread, but verify that you still have sufficient logging for troubleshooting if needed.
🏁 Script executed:
#!/bin/bash # Search for other packet logging in the protocol worker echo "Checking for packet logging in protocol worker files:" fd -e ts -e js --exec grep -l "packet" {} \; | grep -i "protocol" echo "Checking for debug or logging functions in protocol worker:" fd -e ts -e js --exec grep -l "log\|debug\|console" {} \; | grep -i "protocol"Length of output: 667
Validate packet logging removal and confirm logging coverage
The assignment of
lastPacketinsrc/index.tshas been commented out:// lastPacket = fullBuffer.toString()Packet logging now lives in your protocol worker modules, but please verify that you still have enough visibility for troubleshooting:
• Files referencing “packet” in the protocol worker area:
– src/protocolWorker/protocolMain.ts
– src/protocolWorker/protocol.worker.ts
– src/mineflayer/mc-protocol.ts• Files containing other log/debug calls in protocol-related code:
– src/protocolWorker/protocolMain.ts
– src/protocolWorker/protocol.worker.ts
– src/protocolWorker/microsoftAuthflow.ts
– src/mineflayer/minecraft-protocol-extra.tsEnsure these logs surface the details you need when diagnosing protocol issues. If you require more granular packet dumps or error tracing, consider reintroducing targeted logging in the worker.
| @@ -242,14 +236,13 @@ export async function connect (connectOptions: ConnectOptions) { | |||
| }) | |||
| } | |||
| } | |||
| let lastPacket = undefined as string | undefined | |||
| const lastPacket = undefined as string | undefined | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify the impact of lastPacket removal.
The lastPacket variable is now a constant with undefined value, and its assignment at line 535 is commented out. However, it's still referenced in the onPossibleErrorDisconnect function (lines 241-243), which will never enter the conditional block.
-const lastPacket = undefined as string | undefined
+// Remove this constant since it's no longer usedAnd update the error handling function:
const onPossibleErrorDisconnect = () => {
- if (lastPacket && bot?._client && bot._client.state !== states.PLAY) {
+ if (bot?._client && bot._client.state !== states.PLAY) {
- appStatusState.descriptionHint = `Last Server Packet: ${lastPacket}`
+ appStatusState.descriptionHint = `Disconnected before entering play state`
}
}📝 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 lastPacket = undefined as string | undefined | |
| // In src/index.ts around line 239, remove the unused `lastPacket` declaration: | |
| // Remove this constant since it's no longer used | |
| // …later in the same file, update the error‐disconnect handler: | |
| const onPossibleErrorDisconnect = () => { | |
| if (bot?._client && bot._client.state !== states.PLAY) { | |
| appStatusState.descriptionHint = `Disconnected before entering play state` | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/protocolWorker/protocolMain.ts (2)
50-50:⚠️ Potential issueUndefined
botreference.The code checks
bot?._clientbut thebotvariable is not declared or imported in this file. This may cause errors or rely on a global variable.If
botis intended to be a global variable, explicitly access it throughglobalThis.botorwindow.bot. Alternatively, passbotas a parameter to functions that need it.- if (bot?._client) return bot._client + if (globalThis.bot?._client) return globalThis.bot._client
57-65:⚠️ Potential issueUnclear purpose of setTimeout and undefined
botreferences.The setTimeout is used without a delay parameter (defaults to 0) and references the
botvariable that isn't defined in this scope.- setTimeout(() => { + // Execute after the current call stack completes + queueMicrotask(() => { - if (bot) { + if (globalThis.bot) { - bot.on('end', () => { + globalThis.bot.on('end', () => { worker.terminate() }) } else { worker.terminate() } - }) + })
🧹 Nitpick comments (4)
src/protocolWorker/protocolMain.ts (4)
11-12: Consider addressing the TypeScript error instead of suppressing it.The
@ts-expect-errorannotation suppresses type-checking errors instead of addressing the underlying issue. This could potentially hide actual problems in the codebase.Consider properly typing the worker import or documenting the reason for the error suppression if it's unavoidable.
22-33: Improve thecopyPrimitiveValuesutility function.The current implementation has several limitations:
- Uses
anytyping instead of more specific types- Doesn't handle arrays
- Doesn't pass
ignoreKeysparameter to recursive calls-const copyPrimitiveValues = (obj: any, deep = false, ignoreKeys: string[] = []) => { +const copyPrimitiveValues = <T extends Record<string, unknown>>(obj: T, deep = false, ignoreKeys: string[] = []): Partial<T> => { - const copy = {} as Record<string, any> + const copy = {} as Partial<T> for (const key in obj) { if (ignoreKeys.includes(key)) continue if (typeof obj[key] === 'object' && obj[key] !== null && deep) { - copy[key] = copyPrimitiveValues(obj[key]) + if (Array.isArray(obj[key])) { + copy[key] = (obj[key] as unknown[]).map(item => + typeof item === 'object' && item !== null + ? copyPrimitiveValues(item as Record<string, unknown>, deep, ignoreKeys) + : item + ) as T[Extract<keyof T, string>] + } else { + copy[key] = copyPrimitiveValues(obj[key] as Record<string, unknown>, deep, ignoreKeys) as T[Extract<keyof T, string>] + } } else if (typeof obj[key] === 'number' || typeof obj[key] === 'string' || typeof obj[key] === 'boolean') { copy[key] = obj[key] } } return copy }
175-175: JSON serialization may lose data for complex objects.Using
JSON.parse(JSON.stringify(args))can cause data loss for values that don't serialize well to JSON (e.g., functions, symbols, undefined, recursive structures).Consider using a more robust serialization method that handles complex objects better, or document the limitations of what types of arguments can be passed to these methods.
- args: JSON.parse(JSON.stringify(args)) + // Only pass primitive values and simple objects that can be serialized + args: args.map(arg => { + if (arg === undefined || typeof arg === 'function' || typeof arg === 'symbol') { + console.warn('Non-serializable value detected:', typeof arg); + return null; // Or some other placeholder value + } + return arg; + })
203-211: Remove or document commented-out code.Commented-out code adds noise and can confuse future developers. If this is kept for reference, add a comment explaining why.
Either remove the commented-out code or add a comment explaining its purpose:
return eventEmitter - // return new Proxy(eventEmitter, { - // get (target, prop) { - // if (!(prop in target)) { - // // console.warn(`Accessing non-existent property "${String(prop)}" on event emitter`) - // } - // const value = target[prop] - // return typeof value === 'function' ? value.bind(target) : value - // } - // }) + /* + * Proxy implementation removed for performance reasons. + * It was originally used to bind methods and provide warnings for non-existent properties. + */
| if (!window.lastStatsUpdate) { | ||
| window.lastStatsUpdate = Date.now() | ||
| setInterval(() => { | ||
| // Sort by total processing time | ||
| window.packetsThreadBlocking = Object.entries(window.packetTimings) | ||
| .sort(([, a], [, b]) => b.total - a.total) | ||
| .reduce((acc, [key, value]) => { | ||
| acc[key] = value | ||
| return acc | ||
| }, {}) | ||
|
|
||
| // Reset timings for next interval | ||
| window.packetTimings = {} | ||
| window.lastStatsUpdate = Date.now() | ||
| }, 1000) | ||
| } |
There was a problem hiding this comment.
Interval created inside event handler may lead to multiple intervals.
The interval for updating packet statistics is created inside the event handler, which means it could be created multiple times if multiple packets are received before the first interval executes.
- // Update packetsThreadBlocking every second
- if (!window.lastStatsUpdate) {
- window.lastStatsUpdate = Date.now()
- setInterval(() => {
- // Sort by total processing time
- window.packetsThreadBlocking = Object.entries(window.packetTimings)
- .sort(([, a], [, b]) => b.total - a.total)
- .reduce((acc, [key, value]) => {
- acc[key] = value
- return acc
- }, {})
-
- // Reset timings for next interval
- window.packetTimings = {}
- window.lastStatsUpdate = Date.now()
- }, 1000)
- }Move this setup code outside the event handler, possibly at the top of the file or in an initialization function:
// Add at the top level of the file
// Initialize packet statistics tracking
if (!window.packetTimingsIntervalSet) {
window.packetTimingsIntervalSet = true;
window.lastStatsUpdate = Date.now();
window.packetTimings = {};
setInterval(() => {
// Sort by total processing time
window.packetsThreadBlocking = Object.entries(window.packetTimings || {})
.sort(([, a], [, b]) => b.total - a.total)
.reduce((acc, [key, value]) => {
acc[key] = value
return acc
}, {});
// Reset timings for next interval
window.packetTimings = {};
window.lastStatsUpdate = Date.now();
}, 1000);
}| if (data.type === 'properties') { | ||
| // eslint-disable-next-line guard-for-in | ||
| for (const property in data.properties) { | ||
| eventEmitter[property] = data.properties[property] | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
For-in loop without hasOwnProperty check risks prototype pollution.
The ESLint rule is disabled, but it's better to properly guard against prototype pollution by using hasOwnProperty checks or alternative methods.
if (data.type === 'properties') {
- // eslint-disable-next-line guard-for-in
- for (const property in data.properties) {
+ for (const property in data.properties) {
+ if (Object.prototype.hasOwnProperty.call(data.properties, property)) {
eventEmitter[property] = data.properties[property]
+ }
}
}Alternatively, you can use:
Object.keys(data.properties).forEach(property => {
eventEmitter[property] = data.properties[property];
});| if (window.stopPacketsProcessing === true || (Array.isArray(window.stopPacketsProcessing) && window.stopPacketsProcessing.includes(packetMeta.name))) { | ||
| if (window.skipPackets && !window.skipPackets.includes(packetMeta.name)) { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Undeclared global variables create implicit dependencies.
The code references window.stopPacketsProcessing and window.skipPackets which aren't declared in this file, creating implicit dependencies.
Consider defining these global variables somewhere in the codebase or using a more explicit approach to feature flags. Run this script to check if these variables are defined elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for declarations of stopPacketsProcessing and skipPackets
echo "Searching for stopPacketsProcessing declarations:"
rg -A 2 "window\.stopPacketsProcessing|stopPacketsProcessing\s*="
echo -e "\nSearching for skipPackets declarations:"
rg -A 2 "window\.skipPackets|skipPackets\s*="Length of output: 1183
Define and Declare Window Globals
The properties window.stopPacketsProcessing and window.skipPackets are used in two files without ever being declared, leading to implicit dependencies and potential runtime/type errors. Please explicitly declare and initialize these globals:
• src/protocolWorker/protocolMain.ts (lines 86–90)
• src/protocolWorker/protocol.worker.ts
Suggested fixes:
- Create or update a global declarations file (e.g.
src/global.d.ts) and augment the Window interface:
declare global {
interface Window {
/** When true or an array of packet names, stops processing matching packets */
stopPacketsProcessing: boolean | string[];
/** List of packet names to skip when stopPacketsProcessing is active */
skipPackets: string[];
}
}
export {};- Initialize defaults at application startup (e.g. in your entry point):
window.stopPacketsProcessing = false;
window.skipPackets = [];Or pass these flags explicitly (e.g. via constructor arguments or a config object) instead of relying on undeclared globals.
| const eventEmitter = new EventEmitter() as any | ||
| eventEmitter.version = this.version | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unclear this context in createMinecraftProtocolClient.
The this.version reference is ambiguous as it depends on how the function is called and what this references at that point.
const eventEmitter = new EventEmitter() as any
- eventEmitter.version = this.version
+ // Explicitly document where this.version comes from and ensure it exists
+ eventEmitter.version = this.version || 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.
| const eventEmitter = new EventEmitter() as any | |
| eventEmitter.version = this.version | |
| const eventEmitter = new EventEmitter() as any | |
| // Explicitly document where this.version comes from and ensure it exists | |
| eventEmitter.version = this.version || null |
This reverts commit cd84acb.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/protocolWorker/protocolMain.ts (5)
46-47:⚠️ Potential issueUndefined
botreference
This code checksbot?._clientbut doesn't declarebothere. If it's intended to be a global, explicitly readglobalThis.botor passbotas an argument to avoid confusion and potential runtime errors.- if (bot?._client) return bot._client + if (globalThis.bot?._client) return globalThis.bot._client
75-77:⚠️ Potential issueUnclear this context in createMinecraftProtocolClient.
The
this.versionreference is ambiguous as it depends on how the function is called and whatthisreferences at that point.const eventEmitter = new EventEmitter() as any - eventEmitter.version = this.version + // Explicitly document where this.version comes from and ensure it exists + eventEmitter.version = this.version || null
83-87:⚠️ Potential issueDefine and Declare Window Globals
The properties
window.stopPacketsProcessingandwindow.skipPacketsare used without ever being declared, leading to implicit dependencies and potential runtime/type errors.+// Add explicit type checking +if (typeof window.stopPacketsProcessing !== 'undefined' && (window.stopPacketsProcessing === true || (Array.isArray(window.stopPacketsProcessing) && window.stopPacketsProcessing.includes(packetMeta.name)))) {
135-150:⚠️ Potential issueInterval created inside event handler may lead to multiple intervals.
The interval for updating packet statistics is created inside the event handler, which means it could be created multiple times if multiple packets are received before the first interval executes.
Move this setup code outside the event handler, possibly at the top of the file or in an initialization function:
// Add at the top level of the file // Initialize packet statistics tracking if (!window.packetTimingsIntervalSet) { window.packetTimingsIntervalSet = true; window.lastStatsUpdate = Date.now(); window.packetTimings = {}; setInterval(() => { // Sort by total processing time window.packetsThreadBlocking = Object.entries(window.packetTimings || {}) .sort(([, a], [, b]) => b.total - a.total) .reduce((acc, [key, value]) => { acc[key] = value return acc }, {}); // Reset timings for next interval window.packetTimings = {}; window.lastStatsUpdate = Date.now(); }, 1000); }
154-159: 🛠️ Refactor suggestionFor-in loop without hasOwnProperty check risks prototype pollution.
The ESLint rule is disabled, but it's better to properly guard against prototype pollution by using hasOwnProperty checks or alternative methods.
if (data.type === 'properties') { - // eslint-disable-next-line guard-for-in - for (const property in data.properties) { + for (const property in data.properties) { + if (Object.prototype.hasOwnProperty.call(data.properties, property)) { eventEmitter[property] = data.properties[property] + } } }Alternatively, you can use:
Object.keys(data.properties).forEach(property => { eventEmitter[property] = data.properties[property]; });
🧹 Nitpick comments (2)
src/protocolWorker/protocolMain.ts (2)
19-30: Consider adding type safety to copyPrimitiveValues functionThe utility function is useful, but it could benefit from better type safety. The
anyreturn type and parameter should be more specific.-const copyPrimitiveValues = (obj: any, deep = false, ignoreKeys: string[] = []) => { +const copyPrimitiveValues = <T extends Record<string, unknown>>(obj: T, deep = false, ignoreKeys: string[] = []): Partial<T> => { - const copy = {} as Record<string, any> + const copy = {} as Partial<T>
199-209: Remove commented codeThere's commented-out code that appears to be a Proxy implementation. Clean up the codebase by removing or documenting why this code is kept.
return eventEmitter - // return new Proxy(eventEmitter, { - // get (target, prop) { - // if (!(prop in target)) { - // // console.warn(`Accessing non-existent property "${String(prop)}" on event emitter`) - // } - // const value = target[prop] - // return typeof value === 'function' ? value.bind(target) : value - // } - // })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/protocolWorker/protocolMain.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/protocolWorker/protocolMain.ts (6)
src/protocolWorker/protocol.worker.ts (1)
PROXY_WORKER_TYPE(36-135)src/connect.ts (1)
ConnectOptions(13-42)src/parseServerAddress.ts (1)
ParsedServerAddress(48-54)src/protocolWorker/microsoftAuthflow.ts (2)
getAuthData(20-113)authFlowMainThread(115-164)src/appStatus.ts (1)
setLoadingScreenStatus(5-35)renderer/viewer/lib/workerProxy.ts (1)
useWorkerProxy(32-76)
🔇 Additional comments (2)
src/protocolWorker/protocolMain.ts (2)
1-18: Nice module structure and imports setupThe imports are well-organized and the module exports a clear interface for accessing the protocol worker channel. Good practice to expose this as a singleton through a getter function.
193-197: Good authentication flow integrationThe integration with the Microsoft authentication flow is well-implemented. Good use of the
authFlowMainThreadfunction with the event emitter for handling logins.
| const redirectMethodsToWorker = (names: string[]) => { | ||
| for (const name of names) { | ||
| // eslint-disable-next-line @typescript-eslint/no-loop-func | ||
| eventEmitter[name] = async (...args: any[]) => { | ||
| protocolWorkerChannel?.call({ | ||
| name, | ||
| args: JSON.parse(JSON.stringify(args)) | ||
| }) | ||
|
|
||
| if (name === 'write') { | ||
| eventEmitter.emit('writePacket', ...args) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve JSON serialization and handle errors in redirectMethodsToWorker
Using JSON.parse(JSON.stringify(args)) for cloning arguments will fail with circular references, functions, and undefined values. Also, errors during worker communication are not handled.
eventEmitter[name] = async (...args: any[]) => {
try {
- protocolWorkerChannel?.call({
+ await protocolWorkerChannel?.call({
name,
- args: JSON.parse(JSON.stringify(args))
+ args: args.map(arg =>
+ typeof arg === 'object' && arg !== null ?
+ structuredClone(arg) : arg)
})
+ } catch (error) {
+ console.error(`Error calling worker method ${name}:`, error);
+ eventEmitter.emit('error', error);
+ }
if (name === 'write') {
eventEmitter.emit('writePacket', ...args)
}
}📝 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 redirectMethodsToWorker = (names: string[]) => { | |
| for (const name of names) { | |
| // eslint-disable-next-line @typescript-eslint/no-loop-func | |
| eventEmitter[name] = async (...args: any[]) => { | |
| protocolWorkerChannel?.call({ | |
| name, | |
| args: JSON.parse(JSON.stringify(args)) | |
| }) | |
| if (name === 'write') { | |
| eventEmitter.emit('writePacket', ...args) | |
| } | |
| } | |
| } | |
| } | |
| const redirectMethodsToWorker = (names: string[]) => { | |
| for (const name of names) { | |
| // eslint-disable-next-line @typescript-eslint/no-loop-func | |
| eventEmitter[name] = async (...args: any[]) => { | |
| try { | |
| await protocolWorkerChannel?.call({ | |
| name, | |
| args: args.map(arg => | |
| typeof arg === 'object' && arg !== null | |
| ? structuredClone(arg) | |
| : arg | |
| ) | |
| }) | |
| } catch (error) { | |
| console.error(`Error calling worker method ${name}:`, error) | |
| eventEmitter.emit('error', error) | |
| } | |
| if (name === 'write') { | |
| eventEmitter.emit('writePacket', ...args) | |
| } | |
| } | |
| } | |
| } |
| const patchUint8Arrays = (obj: any) => { | ||
| for (const key in obj) { | ||
| if (obj[key] instanceof Uint8Array) { | ||
| obj[key] = Buffer.from(obj[key]) | ||
| } else if (typeof obj[key] === 'object' && obj[key] !== null) { | ||
| patchUint8Arrays(obj[key]) | ||
| } | ||
| } | ||
| } | ||
| patchUint8Arrays(packetData) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the recursive patchUint8Arrays function with explicit checks
The recursive function lacks explicit checks for circular references, which could lead to stack overflow errors with certain data structures.
-const patchUint8Arrays = (obj: any) => {
+const patchUint8Arrays = (obj: any, visited = new Set()) => {
+ if (visited.has(obj)) return; // Prevent circular reference issues
+ visited.add(obj);
for (const key in obj) {
if (obj[key] instanceof Uint8Array) {
obj[key] = Buffer.from(obj[key])
} else if (typeof obj[key] === 'object' && obj[key] !== null) {
- patchUint8Arrays(obj[key])
+ patchUint8Arrays(obj[key], visited)
}
}
}📝 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 patchUint8Arrays = (obj: any) => { | |
| for (const key in obj) { | |
| if (obj[key] instanceof Uint8Array) { | |
| obj[key] = Buffer.from(obj[key]) | |
| } else if (typeof obj[key] === 'object' && obj[key] !== null) { | |
| patchUint8Arrays(obj[key]) | |
| } | |
| } | |
| } | |
| patchUint8Arrays(packetData) | |
| } | |
| const patchUint8Arrays = (obj: any, visited = new Set()) => { | |
| if (visited.has(obj)) return; // Prevent circular reference issues | |
| visited.add(obj); | |
| for (const key in obj) { | |
| if (obj[key] instanceof Uint8Array) { | |
| obj[key] = Buffer.from(obj[key]) | |
| } else if (typeof obj[key] === 'object' && obj[key] !== null) { | |
| patchUint8Arrays(obj[key], visited) | |
| } | |
| } | |
| } | |
| patchUint8Arrays(packetData) |
PR Type
+10% perf on high load servers (1k packets from server) like hypixel: packet serialization / deserialization now happens in its own thread
Enhancement, Bug fix
Description
Introduced a new
protocolWorkermodule for handling Minecraft protocol tasks.protocolMain.tsandprotocolWorker.tsfor managing protocol-related operations.Refactored authentication flow to streamline Microsoft account integration.
microsoftAuthflowlogic toprotocolWorker/microsoftAuthflow.authFlowMainThreadandgetAuthDatafor improved token management.Improved version handling and utility functions.
toMajorVersionutility to a more appropriate location.Removed unused or redundant code for better maintainability.
Changes walkthrough 📝
9 files
Added `toMajorVersion` utility functionUpdated imports to include `toMajorVersion`Refactored connection logic and removed unused codeRefactored Microsoft authentication flowAdded main protocol handling logicIntroduced worker for protocol-related tasksUpdated imports for `toMajorVersion`Removed `toMajorVersion` and cleaned up utilitiesUpdated imports for refactored authentication flow1 files
Simplified packet validation logic1 files
Added global configurations for protocol workerSummary by CodeRabbit
New Features
Refactor