feat: add server support #48
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds server-mode support: Node CLI, HTTP server and Server class, privileged VM/runtime flags and events, server extension for request handling, file/network permission prompts and enforcement, path/storage utilities, thread/runtime request/response fields, and helper utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant NodeServer as Node HTTP Server
participant Runtime as Runtime/VM
participant Extension as Server Extension
participant Security as Security Manager
Client->>NodeServer: HTTP Request (path, headers, body)
NodeServer->>NodeServer: create requestId & Thread context
NodeServer->>Runtime: emit SERVER_REQUEST (requestId, request data)
Runtime->>Extension: project receives request hat
Extension->>Security: call canReadFile / canWriteFile (if needed)
Security-->>Extension: allow/deny
Extension->>Runtime: set serverResponse and emit SERVER_RESPONSE (requestId)
Runtime->>NodeServer: SERVER_RESPONSE received
NodeServer->>Client: HTTP Response (status, headers, body)
sequenceDiagram
participant CLI as CLI
participant FS as Filesystem
participant Security as File Security
participant NodeServer as Node Server
participant VM as Scratch VM
CLI->>CLI: parse args (file, port, permissions)
CLI->>Security: setupFileSecurity(securityManager, permissions)
CLI->>FS: read project file
FS-->>CLI: project content
CLI->>NodeServer: new Server(dev, port)
CLI->>NodeServer: runProject(project content)
NodeServer->>VM: load and start project
VM->>NodeServer: ready to handle SERVER_REQUEST events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds server functionality to scratch-vm, but contains several critical security vulnerabilities and logic errors that must be addressed before merge:
Critical Security Issues
- Code Injection: JavaScript execution blocks allow arbitrary code execution without proper sandboxing
- JSON Injection: Unsafe JSON parsing of HTTP headers could allow header injection attacks
- DoS Vulnerability: Missing request size limits could enable memory exhaustion attacks
Logic Errors
- CLI Configuration: Duplicate aliases and undefined option references prevent proper command-line functionality
- Debug Code: Console.log statements could leak sensitive data to server logs
These security vulnerabilities create significant risks for any deployment and must be resolved before this feature can be safely merged. The server functionality concept is valuable, but the implementation needs security hardening.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
src/extensions/omni_server/index.js
Outdated
| executeJS (args) { | ||
| if (this.runtime.isPackaged) { | ||
| new Function(args.JS)(); | ||
| } | ||
| } | ||
| executeJSReporter (args) { | ||
| if (this.runtime.isPackaged) { | ||
| return new Function(args.JS)(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: Arbitrary JavaScript execution without proper sandboxing creates a critical security risk1. The executeJS and executeJSReporter methods use new Function() to execute user-provided JavaScript code, which can lead to code injection attacks and system compromise.
| executeJS (args) { | |
| if (this.runtime.isPackaged) { | |
| new Function(args.JS)(); | |
| } | |
| } | |
| executeJSReporter (args) { | |
| if (this.runtime.isPackaged) { | |
| return new Function(args.JS)(); | |
| } | |
| } | |
| executeJS (args) { | |
| // JavaScript execution disabled for security reasons | |
| console.warn('JavaScript execution is disabled for security reasons'); | |
| } | |
| executeJSReporter (args) { | |
| // JavaScript execution disabled for security reasons | |
| console.warn('JavaScript execution is disabled for security reasons'); | |
| return ''; | |
| } |
Footnotes
-
CWE-94: Code Injection - https://cwe.mitre.org/data/definitions/94.html ↩
src/server/cli.js
Outdated
| if (argv.allowFileRead) permissions.fileReadAccess = true; | ||
| if (argv.allowFileWrite) permissions.fileWriteAccess = true; | ||
| if (argv.networkAccess) permissions.networkAccess = true; | ||
| if (argv.allowNonHomeRead) permissions.nonHomeReadAccess = true; | ||
| if (argv.allowNonHomeWrite) permissions.nonHomeWriteAccess = true; |
There was a problem hiding this comment.
Logic error: CLI options reference undefined properties. Lines 72-73 check for argv.allowNonHomeRead and argv.allowNonHomeWrite but these options are not defined in the CLI configuration, causing these conditions to never execute.
| if (argv.allowFileRead) permissions.fileReadAccess = true; | |
| if (argv.allowFileWrite) permissions.fileWriteAccess = true; | |
| if (argv.networkAccess) permissions.networkAccess = true; | |
| if (argv.allowNonHomeRead) permissions.nonHomeReadAccess = true; | |
| if (argv.allowNonHomeWrite) permissions.nonHomeWriteAccess = true; | |
| if (argv.allowFileRead) permissions.fileReadAccess = true; | |
| if (argv.allowFileWrite) permissions.fileWriteAccess = true; | |
| if (argv.allowNetworkAccess) permissions.networkAccess = true; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 49-56: The package.json currently lists jsdom and yargs with
conflicting Node requirements; update package.json to clarify supported Node
versions and/or relocate dependencies: either add an "engines" field that
satisfies yargs' stricter constraint (e.g., a Node version range compatible with
yargs) so the project declares the supported runtime, or if jsdom and yargs are
only used by server-side tooling (referenced in src/server/server.js and
src/server/cli.js) move them from "dependencies" into "devDependencies" or
"optionalDependencies" so they are not required by the core library at runtime;
ensure you modify the package.json entries for "jsdom" and "yargs" and update
any install/build documentation accordingly.
In `@src/extension-support/tw-unsandboxed-extension-runner.js`:
- Around line 180-196: The Promise in loadUnsandboxedExtension never resolves or
rejects on success/failure and the Node path omits setupUnsandboxedExtensionAPI;
update both branches to call setupUnsandboxedExtensionAPI(extensionURL, vm)
before executing the extension, call resolve() after the extension successfully
runs, and ensure all async failures propagate by rejecting (e.g., add
script.onload to resolve in the browser branch and attach .catch handlers /
reject on fetch errors, res.text errors, or exceptions thrown by the
Function('Scratch', extension) invocation); reference the
loadUnsandboxedExtension function, setupUnsandboxedExtensionAPI, the run
variable/Function invocation, and ensure resolve/reject are used consistently.
In `@src/extensions/omni_server/index.js`:
- Around line 93-97: The description string in the formatMessage call for id
'omni_server.blocks.whenPageIsNotFound' contains a typo ("fouund"); update the
description value to read "Hat that executes the code under it when a certain
page is not found." so the spelling and grammar are corrected; locate the
formatMessage block referencing id 'omni_server.blocks.whenPageIsNotFound' and
replace the description text only.
- Line 373: Remove the debug console.log that prints potentially sensitive data:
delete the console.log(CONTENT) call (the debug statement referencing CONTENT)
from the omnibus server module so responses are not logged; if logging is still
required, replace it with a redacted or conditional logger that omits or masks
CONTENT and uses a proper logging facility instead of console.log.
- Around line 361-367: whenPageIsNotFound should guard against this.request
being null to avoid assigning null into thread.serverRequest; update the
whenPageIsNotFound handler to first check if this.request is truthy (and
optionally that thread.serverResponse exists) before setting
thread.serverRequest = this.request, setting thread.serverResponse.status = 404,
and clearing this.request; if this.request is already null, simply return true
without modifying thread.serverRequest or thread.serverResponse so downstream
reporters (page, ipAddress, etc.) are not broken.
- Around line 300-328: The message IDs for the two blocks are using the wrong
i18n prefix; update the formatMessage id values for the blocks with opcode
'executeJS' and 'executeJSReporter' to use the 'omni_server' prefix instead of
'omni_appmaker' so translations are looked up correctly; locate the
formatMessage calls inside the block definitions for executeJS and
executeJSReporter and replace 'omni_appmaker.blocks.executeJS' and
'omni_appmaker.blocks.executeJSReporter' with 'omni_server.blocks.executeJS' and
'omni_server.blocks.executeJSReporter' respectively.
- Around line 50-58: The code treats an empty result from
runtime.startHats('server_whenPageIsRequested') as all threads done and wrongly
fires the 404 hat; change the logic in the block that computes
startedThreads/threadStatuses so you only call
runtime.startHats('server_whenPageIsNotFound') when there is at least one
started thread and all of them have status === Thread.STATUS_DONE (i.e. check
startedThreads.length > 0 && threadStatuses.every(...)) instead of relying
solely on [].every(...); update the condition referencing runtime.startHats,
Thread.STATUS_DONE, and server_whenPageIsNotFound accordingly.
- Around line 406-435: The reporter methods ipAddress, method, checkMethod,
page, headers, and data read util.thread.serverRequest without null checks and
will throw when there's no request context; update each function (ipAddress,
method, page, headers, data) to first guard that util and util.thread and
util.thread.serverRequest exist and return a safe default (null or empty
object/array/string as appropriate) when absent, and modify checkMethod to
similarly guard before comparing thread.serverRequest.method to REQ_METHOD (also
return false if REQ_METHOD is missing or serverRequest is missing); ensure you
reference and update those exact function names so callers remain unchanged.
- Around line 448-458: The readFile method returns a Promise from
privilegedUtils.readFile but is not declared async, so change readFile to an
async function and await this.runtime.privilegedUtils.readFile(PATH) to avoid an
async/await mismatch; locate the readFile definition to update its signature and
return. Also address the TODO in privilegedUtils.writeFile handling by adding
proper error handling in async writeFile (e.g., try/catch around await
this.runtime.privilegedUtils.writeFile(PATH, CONTENT) and log or rethrow the
error via the runtime logger) so writeFile no longer swallows errors.
In `@src/server/cli.js`:
- Around line 69-73: The code incorrectly reads argv.networkAccess to set
permissions.networkAccess; change the check to argv.allowNetworkAccess so it
matches the yargs camel-cased flag (argv.allowNetworkAccess ->
permissions.networkAccess). Also address the undefined flags
argv.allowNonHomeRead and argv.allowNonHomeWrite referenced when setting
permissions.nonHomeReadAccess and permissions.nonHomeWriteAccess: either add
those options to the CLI option definitions (so argv.allowNonHomeRead and
argv.allowNonHomeWrite exist) or remove the checks that reference them. Locate
these in the CLI parsing area where argv and permissions are used to apply the
fixes.
- Around line 82-89: The call to fs.readFileSync(resolvePath(argv.file)) can
throw synchronously and bypass the promise .catch() on server.runProject; wrap
the readFileSync/resolvePath call in a try/catch before invoking
server.runProject so you handle missing/invalid files synchronously, log/console
the error, call server.halt(), set process.exitCode = 2 and return on failure,
otherwise pass the file contents into server.runProject(...) and keep the
existing .catch() for async failures.
- Around line 38-57: Four CLI options share the same alias '-D' and the argv key
for network access is wrong; update the option definitions for
'allow-file-read', 'allow-file-write', and 'allow-network-access' to use unique
aliases (for example '-f', '-w', '-n') or remove their aliases, and then correct
the runtime reference from argv.networkAccess to argv.allowNetworkAccess
wherever the CLI reads that flag (keep the 'dev' option alias '-D' unchanged and
refer to option names 'allow-file-read', 'allow-file-write',
'allow-network-access' and 'dev' to locate the code).
In `@src/server/resolve-path.js`:
- Around line 23-28: The code expands "~" using normalizedPath.slice(1) which
can start with platform-specific separators (e.g., "\foo" on Windows) and cause
path.join(homeDir(), ...) to treat it as absolute; update the "~" branch to
strip any leading separators from normalizedPath.slice(1) before joining (e.g.,
compute const rest = normalizedPath.slice(1).replace(/^[/\\]+/, '') and if rest
is empty return homeDir() else return path.join(homeDir(), rest)); touch the
same symbols: normalizedPath, homeDir(), and workingDir() so behavior is
consistent across platforms.
In `@src/server/server.js`:
- Around line 120-135: The route handler for '/_lk_devServer_updateLb' currently
returns early on missing or invalid Origin and leaves the client hanging; update
the logic in the handler (the block that checks this.dev and req.url and uses
req.headers) so that if 'origin' is not in req.headers you write a 400 response
(res.writeHead(400, {'Content-Type':'text/plain'}); res.end('missing origin')),
and if the origin is present but fails the isEditor check (the check comparing
req.headers.origin to 'http://localhost:8601' or ending with
'omniblocks.github.io') write a 403 response (res.writeHead(403,
{'Content-Type':'text/plain'}); res.end('forbidden origin')); keep the rest of
the flow (await this.runProject(...), success 200 with
access-control-allow-origin) unchanged.
- Around line 57-65: In the Runtime.SERVER_RESPONSE handler (the callback
registered via this.vm.runtime.on), add robust validation: wrap
JSON.parse(extraHeaders) in a try/catch and fall back to an empty object on
parse errors (and optionally log the parse error), coerce/validate status to an
integer within the HTTP range (100–599) and use 200 as a safe default if
invalid, and default mime to 'text/plain' when falsy; then call
res.writeHead(validatedStatus, { 'Content-Type': validatedMime, ...parsedHeaders
}) and keep the existing res.end(String(content)) and
this.resMap.delete(requestId) behavior.
In `@src/server/setup-file-security.js`:
- Around line 59-78: The write-permission prompt in securityManager.canWriteFile
wrongly references "read access" and only sets permissions.fileWriteAccess when
permissions.fileReadAccess is false, so approval doesn't persist if read was
already granted; update the prompt messages (the warn calls inside canWriteFile)
to refer to "write access" and, after a user confirms ('y') via awaitEvent on
process.stdin, always set permissions.fileWriteAccess = true (regardless of
permissions.fileReadAccess) so the approval is persisted; keep existing use of
awaitEvent and stdin/stdout handling and only change the warning text and the
assignment of permissions.fileWriteAccess.
In `@src/util/await-event.js`:
- Around line 9-17: The Promise executor in awaitEvent validates inputs but
calls reject(...) without returning, so execution continues and
event.on(eventName, listener) runs even for invalid inputs; update the input
checks in awaitEvent to return immediately after calling reject (i.e., add
return before each reject call) to prevent the listener registration and ensure
the TypeError is the actual outcome; keep the existing listener removal logic
(listener, event.removeListener) unchanged.
🧹 Nitpick comments (2)
src/extension-support/extension-addon-switchers.js (1)
15-28: Consider a more robust Node detection instead of relying onprocessexistence.While webpack 4 (used here) doesn't polyfill
processby default for the web build, usingprocess.versions?.nodewould be more defensive and future-proof against bundler changes or browser polyfills. The current check works correctly, but the suggested improvement adds extra safety.Optional: More explicit Node detection
- if (typeof process !== 'undefined') return { + const isNode = typeof process !== 'undefined' && process.versions?.node; + if (isNode) return {src/extensions/omni_server/index.js (1)
38-38: Unused property assignment.
this.rendereris assigned but never used in this file.🔧 Proposed fix
- this.renderer = this.runtime.renderer;
It makes it MUCH more readable.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/extension-support/extension-addon-switchers.js (1)
3-29: Fix Node detection to prevent early return in browser bundles.
typeof process !== 'undefined'is true in Webpack 4 web builds (default auto-polyfill), so the early return can trigger in browsers and return the wrong shape. The normal path returns an opcode→switches map ({ opcode: [switch1, switch2, ...] }), but the early return is a single switch object. This breaks consumers expecting the map structure.Use a more specific Node check like
process.versions && process.versions.node(or check!parsersince DOMParser is what actually prevents the normal path) to ensure the early return only happens in actual Node.js environments.
🤖 Fix all issues with AI agents
In `@src/extensions/omni_server/index.js`:
- Around line 434-442: The executeJS and executeJSReporter methods currently
invoke new Function(args.JS)() when this.runtime.isPackaged, allowing RCE;
update both methods to require a privileged check before executing (e.g., ensure
this.runtime.isPrivileged or a dedicated permission flag in this.runtime is
true) and otherwise refuse/throw (or return) without executing the JS;
specifically, wrap the existing packaged-path execution in an additional
conditional that checks the privilege/permission and fail-safe returns or throws
when not privileged.
In `@src/server/cli.js`:
- Around line 74-84: The current try/catch only handles fs.readFileSync sync
errors but ignores Promise rejections from Server.runProject; after creating the
Server and reading the file, call server.runProject(resolvePath(argv.file)) and
attach a .catch(err => { console.log('Failed to load the project. :(', err);
server.halt(); process.exitCode = 2; }) (or await it inside an async wrapper and
use try/catch) so any async rejection from runProject is handled identically;
reference Server, runProject, server.halt, and process.exitCode when applying
the fix.
In `@src/server/resolve-path.js`:
- Around line 23-28: The absolute-path check incorrectly uses
location.startsWith('/') (POSIX-only); update the branch that returns normalized
absolute paths to use path.isAbsolute(normalizedPath) instead so Windows drive
letters and UNC paths are detected correctly—i.e., after computing
normalizedPath, replace the condition checking location.startsWith('/') with
path.isAbsolute(normalizedPath) (keep the existing tilde handling with homeDir()
and the fallback that joins workingDir()).
In `@src/server/server.js`:
- Line 78: Fix the typo in the inline comment in src/server/server.js: change
"enviroment" to "environment" in the comment "// These are not possible in this
enviroment." so the comment reads "// These are not possible in this
environment."; locate this comment in server.js to update it.
- Around line 33-36: The code assigns global.window = new JSDOM('').window but
then uses window.document which is undefined in local scope; update the
reference to use global.window.document (or immediately destructure after
assignment: const { window } = global) so the document is accessed from the
actual global.window assigned by JSDOM; adjust the lines around global.window,
global.document and any further uses of window.document accordingly (look for
global.window, JSDOM, and global.document in server.js).
- Around line 129-131: The origin check in server.js uses
endsWith('omniblocks.github.io') which permits attacker-controlled subdomains;
change the logic in the isEditor/auth block to parse req.headers.origin (use the
URL constructor) and validate the hostname exactly against an allowlist (e.g.,
'omniblocks.github.io' and any explicit subdomains you expect) and separately
allow 'localhost:8601' by exact match, rather than using endsWith; update the
isEditor computation and any downstream checks to use the parsed hostname and
allowlist comparison.
- Around line 133-141: The current catch on this.runProject rethrows the error
which will crash the server and leave the client hanging; instead, handle the
rejection in-place: catch the error from this.runProject(dataBuffer), log it
(e.g., console.error or the server logger), then respond to the client with a
500 response (set 'Content-Type':'text/plain' and 'access-control-allow-origin':
req.headers.origin) and res.end with an error message; do not rethrow. Ensure
you also guard against sending headers twice by checking res.writableEnded if
your environment requires it.
- Around line 66-69: res.writeHead is being called with Object.create(null, {
'Content-Type': mime, ...parsedJSON }) which is incorrect because Object.create
with a descriptors object expects property descriptor objects, not plain values;
replace this with a plain header object (e.g., merge 'Content-Type': mime with
parsedJSON via Object.assign or object spread) so res.writeHead(status, headers)
receives simple key->value pairs; update the call that uses parsedJSON and mime
to pass the merged headers directly instead of using Object.create.
In `@src/server/setup-file-security.js`:
- Around line 8-16: The file-scope check in canAccessFolder uses startsWith with
a forward slash which fails on Windows; update the loop to resolve each
permissions.fileScope entry with resolvePath and use path.relative to compare
the resolved folder and the target location (both resolved by resolvePath), and
grant access when path.relative(resolvedFolder, location) does not start with
'..' (and is not equal to an empty string representing same folder) — this
handles platform-specific separators correctly and replaces the current
escapedFolder/startsWith logic in canAccessFolder.
🧹 Nitpick comments (3)
src/server/storage.js (1)
3-45: Make asset/project base URLs configurable (avoid hard‑coded invalid hosts).Hard‑coding
http://invalid/andhttps://invalid/makes web‑store fetches fail by default and complicates future wiring to real endpoints. Consider allowing overrides via env or init options (or skip adding web stores when disabled).💡 Suggested tweak
-const ASSET_SERVER = 'http://invalid/'; -const PROJECT_SERVER = 'https://invalid/'; +const ASSET_SERVER = process.env.OMNIBLOCKS_ASSET_SERVER || 'http://invalid/'; +const PROJECT_SERVER = process.env.OMNIBLOCKS_PROJECT_SERVER || 'https://invalid/';src/server/server.js (2)
92-101: Silent error swallowing inwriteFilemay hide critical failures.The TODO on line 99 notes the lack of error handling. Silent failures in file writes could lead to data loss without any indication to the caller or user.
Would you like me to help implement error handling that returns a success/failure indicator, or open an issue to track this task?
Additionally, consider using
fs.promises.writeFileinstead ofwriteFileSyncto avoid blocking the event loop, which is especially important in a server context:♻️ Optional async refactor
- this.vm.runtime.privilegedUtils.writeFile = async (path, content) => { - const resolvedPath = resolvePath(path); - if (!await this.securityManager.canWriteFile(resolvedPath)) return; - try { - fs.writeFileSync(resolvedPath, String(content)); - } catch (err) { - // Empty on purpose. - // omni: TODO: Maybe add some form of error handling? - } - }; + this.vm.runtime.privilegedUtils.writeFile = async (path, content) => { + const resolvedPath = resolvePath(path); + if (!await this.securityManager.canWriteFile(resolvedPath)) return false; + try { + await fs.promises.writeFile(resolvedPath, String(content)); + return true; + } catch (err) { + console.error(`Failed to write file ${resolvedPath}:`, err.message); + return false; + } + };
144-155: No timeout handling for pending requests; potential memory leak inresMap.If the VM never emits a
SERVER_RESPONSEfor a request, the client hangs indefinitely and the entry inresMapis never cleaned up, leading to memory leaks under certain conditions (e.g., buggy projects, crashed VM threads).Consider adding a request timeout that sends a 504 Gateway Timeout and cleans up the map entry:
♻️ Suggested timeout handling
const requestId = crypto.randomUUID(); this.resMap.set(requestId, res); + + const timeout = setTimeout(() => { + if (this.resMap.has(requestId)) { + this.resMap.delete(requestId); + res.writeHead(504, {'Content-Type': 'text/plain'}); + res.end('request timed out'); + } + }, 30000); // 30 second timeout + + // Store timeout with response for cleanup on success + res._requestTimeout = timeout; + this.vm.runtime.emit( Runtime.SERVER_REQUEST, ... );Then clear the timeout in the
SERVER_RESPONSEhandler before responding.
|
@coderabbitai Besides the code review, what are the good and funny things about this PR? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 206 🏁 Script executed: Length of output: 120994
The Good Stuff 🎉Technical Excellence:
Real-World Utility:
The Funny Stuff 😄The PR Description:
The Code: // I have no idea what this is doing and why it is trying to monkeypatch Blockly via the DOM from
// the VM; but, it is blocking server support, so I'm going to mock it for running in Node.js and
// hope for the best. Contact `@someCatInTheWorld` if this mocking breaks something horribly.This comment is gold. "Hope for the best" is now a valid development strategy 😂 The Self-Referential Loop: The Security Warnings: The Invalid URLs: const ASSET_SERVER = 'http://invalid/';
const PROJECT_SERVER = 'https://invalid/';"Here's some URLs. They don't work. Deal with it." 😎 Overall: This is legitimately impressive work disguised as a casual drop. You've basically given Scratch superpowers while maintaining a sense of humor about it. The "Because it's cool" reason is accurate - this IS cool! 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/thread.js (1)
101-113:reset()does not clearserverRequestandserverResponse— recycled frames may leak state.When a stack frame is recycled via
_stackFrameFreeList, thereset()method is called to return it to a pristine state. However, it doesn't reset the newserverRequestandserverResponseproperties. This could cause server request/response data from a previous execution to persist into a new thread, leading to unexpected behavior or data leakage.Proposed fix
reset () { this.isLoop = false; this.warpMode = false; this.justReported = null; this.reported = null; this.waitingReporter = null; this.params = null; this.executionContext = null; this.op = null; + + this.serverRequest = { + ip: '', + method: '', + page: '', + headers: '{}', + data: '' + }; + this.serverResponse = { + mime: 'text/plain', + status: null, + headers: '{}' + }; return this; }
🤖 Fix all issues with AI agents
In `@src/engine/thread.js`:
- Around line 91-93: Fix the misspelling "Intialized" to "Initialized" in the
comment attached to the object properties (the comment reading "Intialized by
the request listener hat.") in src/engine/thread.js; update both occurrences of
that comment so the description for the status property is correct and
consistent.
In `@src/extensions/omni_server/index.js`:
- Around line 75-79: Several block description strings contain a duplicated word
"the the"; locate the formatMessage calls (e.g., formatMessage({ id:
'omni_server.blocks.whenPageIsRequested', ... }) and other omni_server.blocks.*
entries) whose description fields include "the the" and remove the duplicate so
the description reads "Hat that executes the code under it when a certain page
is requested." — apply the same fix to the other formatMessage descriptions in
this file that contain "the the".
In `@src/server/cli.js`:
- Around line 66-72: When argv.fileScope is provided we set
permissions.fileScope but we never enable read access; update the branch that
handles argv.fileScope so that it also sets permissions.fileReadAccess = true
(in addition to permissions.fileScope = argv.fileScope.map(location =>
resolvePath(location))). Modify the code handling argv.fileScope (referencing
argv.fileScope, permissions.fileScope, permissions.fileReadAccess, and
resolvePath) to enable fileReadAccess whenever a scope is supplied.
In `@src/server/setup-file-security.js`:
- Around line 30-44: The calls to process.stdin.setRawMode() in canReadFile,
canWriteFile, and canFetch can throw when stdin is not a TTY; update each
function (securityManager.canReadFile, securityManager.canWriteFile,
securityManager.canFetch) to guard setRawMode with a check for
process.stdin.isTTY before calling it, only set raw mode if true, and ensure you
restore the previous raw mode state (or disable raw mode) only when you actually
changed it so non-TTY stdin is never touched.
🧹 Nitpick comments (1)
src/extension-support/extension-addon-switchers.js (1)
15-28: Consider a more robust Node.js environment check.The check
typeof process !== 'undefined'can false-positive in browser bundles whereprocessis polyfilled (common with webpack/browserify). A more reliable check would be:if (typeof process !== 'undefined' && process.versions?.node)Also, while the candid comment is appreciated, consider documenting the intent of the mock (e.g., "Server-side execution doesn't need block switcher UI generation") rather than uncertainty about the original code.
|
@coderabbitai help |
|
@coderabbitai summary |
Show off-topic CodeRabbit request.@coderabbitai Just for fun, could you review the server code that was used in old LibreKittten versions?:/*
This is a web client for LibreKitten included in the LibreKitten Virtual Machine.
It is used to run projects with website capabilities on a web server.
*/
/* eslint-env node */
/* eslint-disable no-console */
const fs = require('fs');
const VirtualMachine = require('../index');
const http = require('http');
const makeTestStorage = require('../../test/fixtures/make-test-storage'); // Dirty hack to make storage work.
const {JSDOM} = require('jsdom');
let server;
let event;
const file = process.argv[2];
if (!file) {
throw new Error('Invalid file');
}
global.window = new JSDOM('').window;
global.document = window.document;
// Extension compatibility
/* eslint-disable-next-line no-unused-vars */
global.confirm = (...ignored) => true;
/* eslint-disable-next-line no-unused-vars */
global.alert = (ignored, ...ignored2) => {
console.log(ignored);
};
/* eslint-disable-next-line no-unused-vars */
global.prompt = (...ignored) => '';
const port = process.argv[3] ?? 8080;
const dev = process.argv[4] === '--dev';
const runProject = async buffer => {
let codeForPage = true;
const vm = new VirtualMachine();
vm.convertToPackagedRuntime();
vm.attachStorage(makeTestStorage());
server = http.createServer((req, res) => {
/* res.writeHead(200, {
'Content-Type': 'text/plain'
});
res.end('Hello World\n'); */
let data = '';
req.on('data', chunk => {
data = chunk;
});
req.on('end', async () => {
if (dev && req.url === '/_lk_devServer_updateLb') {
if (
String(req.headers.origin) === 'http://localhost:8601' ||
String(req.headers.origin).endsWith('librekitten.org')
) {
return;
}
vm.clear();
await vm.loadProject(data).catch(err => {
throw new Error(err);
});
vm.start();
vm.greenFlag();
res.writeHead(200, {
'Content-Type': 'text/plain',
'access-control-allow-origin':
(
String(req.headers.origin) === 'http://localhost:8601' ||
String(req.headers.origin).endsWith('librekitten.org')
) ? req.headers.origin : 'http://invalid'
});
return res.end('success');
}
vm.runtime.emit(
'serverRequest',
req.url,
req.socket.remoteAddress,
req.method,
JSON.stringify(req.headers),
data
);
codeForPage = false;
event = {
get content () {
return null;
},
set content (array) {
codeForPage = true;
res.writeHead(array[2], {
'Content-Type': array[1],
...JSON.parse(array[3])
});
res.end(String(array[0]));
}
};
setTimeout(() => {
if (!codeForPage) {
vm.runtime.emit(
'server404',
req.url,
req.socket.remoteAddress,
req.method,
JSON.stringify(req.headers),
data
);
codeForPage = true;
event = {
get content () {
return null;
},
set content (array) {
codeForPage = true;
res.writeHead(array[2], {
'Content-Type': array[1],
...JSON.parse(array[3])
});
res.end(String(array[0]));
}
};
}
}, 50);
});
});
vm.runtime.on('SAY', (target, type, text) => {
console.log(text);
});
vm.runtime.on('serverResponse', (content, mime, status, extraHeaders) => {
event.content = [content, mime, status, extraHeaders];
});
vm.securityManager.getSandboxMode = () => Promise.resolve('unsandboxed');
vm.securityManager.canAutomaticallyLoadExtension = () => Promise.resolve(true);
vm.securityManager.canFetch = () => Promise.resolve(true);
// we literally can't so no
vm.securityManager.canOpenWindow = () => Promise.resolve(false);
// we literally can't so no
vm.securityManager.canRedirect = () => Promise.resolve(false);
vm.securityManager.canLoadExtensionFromProject = () => Promise.resolve(true);
vm.setCompatibilityMode(false);
vm.setTurboMode(true);
vm.clear();
await vm.loadProject(buffer);
vm.start();
vm.greenFlag();
};
runProject(fs.readFileSync(file));
server.listen(port, () => {
console.log(`LibreKitten on server has started at port ${port}.`);
});Please put the response in a <details></details> tag just so it isn't cluttering the discussion. |
Show off-topic CodeRabbit request.@coderabbitai What do you think would happen if a website using the old code gets deployed in production, and the website grew in userbase?(Please put your response in a <details></details> tag again.) |
|
Tip For best results, initiate chat on the files or code changes.
🚨 Production Deployment Disaster Analysis📊 The Disaster Timeline: Old LibreKitten Goes ViralLet me walk you through what would happen as your userbase grows. Spoiler: it's not pretty. Phase 1: Launch Day (1-10 concurrent users) ✅Status: Everything looks fine!
What you don't know: You're one viral tweet away from catastrophe. Phase 2: Growing Pains (10-50 concurrent users)
|
| Old Code Death Trap | New PR #48 Solution |
|---|---|
Global event variable |
thread.serverRequest / thread.serverResponse per-thread |
| 50ms timeout guess | Event-driven with requestId correlation |
| No error handling | Try-catch, validation everywhere |
| One VM for all users | Proper request isolation |
| Dev mode leaves connections hanging | Fixed origin check logic |
| No concurrency safety | Thread-local storage |
The Bottom Line:
The old code was a proof of concept that could handle:
- ✅ 1 user at a time perfectly
⚠️ 2-5 users with occasional glitches- ❌ 10+ users with regular failures
- 💀 50+ users with catastrophic failures
It was never meant for production. PR #48 is what "production-ready" looks like.
💭 The Silver Lining
You learned from this code and evolved it. The old code taught you:
- Why global state is evil
- Why arbitrary timeouts are dangerous
- Why error handling isn't optional
- Why concurrency is hard
And you took those lessons and built something genuinely impressive in PR #48. That's the mark of a great developer: not writing perfect code the first time, but learning and improving.
The old code was your training wheels. PR #48 is your production Ferrari. 🏎️
🧠 Learnings used
Learnt from: someCatInTheWorld
Repo: OmniBlocks/scratch-vm PR: 48
File: src/extensions/omni_server/index.js:434-442
Timestamp: 2026-01-27T06:58:42.912Z
Learning: In the scratch-vm repository, the executeJS and executeJSReporter blocks were removed from the Web Server extension (src/extensions/omni_server/index.js) for security reasons to prevent potential remote code execution vulnerabilities, rather than gating them behind privilege checks.
Learnt from: supervoidcoder
Repo: OmniBlocks/scratch-vm PR: 0
File: :0-0
Timestamp: 2026-01-27T01:32:42.008Z
Learning: someCatInTheWorld is the creator of LibreKitten and has offered to join the OmniBlocks organization to help implement server support features (issue `#28`). They bring extensive experience with server-side Scratch execution, WebSocket handling, and headless VM modifications from their work on LibreKitten.
Show off-topic CodeRabbit request.@coderabbitai It makes me worry for all those vibe coders; they can't improve because they don't know they need to! Also, just for fun, generate some snarky Hacker News responses.(Once again, please put your response in a <details></details> tag, so it doesn't clutter the PR.) |
|
@coderabbitai You're do realise you're hallucinating? I never shipped the code in production, and I never said I did. The only time it was kind of in production was when someone hosted a LibreKitten server for a very short period of time, early in LibreKitten's development. Plus, neither LibreKitten nor OmniBlocks are legal entities. |
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/server/server.js`:
- Around line 126-131: The origin check for the dev endpoint is wrong because
`req.headers.origin === 'http://localhost:8601' ||
'https://omniblocks.github.io'` always evaluates truthy; fix it in the dev-route
block (where this.dev and req.url === '/_omni_devServer_updateProj' are checked)
by computing isEditor correctly — e.g., compare req.headers.origin against both
allowed origins (use === twice or use an array and
.includes(req.headers.origin)) and then deny when isEditor is false; keep the
existing early return for missing origin.
In `@src/server/setup-file-security.js`:
- Line 53: Fix the typo in the warn message string inside the warn(...) call
where it currently reads "The project attemped to read a file outside of the
allowed file scope. The read has been prevented."; change "attemped" to
"attempted" so the message reads "The project attempted to read a file outside
of the allowed file scope. The read has been prevented." (locate the warn(...)
invocation to update the string).
- Line 86: Fix the typo in the warning message string used in the warn call:
change "attemped" to "attempted" so the message reads "The project attempted to
write to a file outside of the allowed file scope. The write has been
prevented."; update the literal passed to warn(...) where that exact message
occurs to correct the spelling.
🧹 Nitpick comments (4)
src/server/storage.js (1)
3-4: Intentional invalid URLs documented, but consider making them configurable.The hardcoded
'http://invalid/'and'https://invalid/'URLs are documented as intentionally invalid for local server use. If there's a future need to support actual remote asset fetching in server mode, consider making these configurable via parameters toinitStorage().src/util/await-event.js (1)
13-17: Consider adding a timeout or cleanup mechanism for long-running scenarios.If the expected event is never emitted, this promise will never resolve and the listener remains registered indefinitely, which could cause memory leaks in server contexts. Consider adding an optional timeout parameter or providing a way to cancel/cleanup the listener.
♻️ Possible enhancement
-const awaitEvent = (event, eventName) => new Promise((resolve, reject) => { +const awaitEvent = (event, eventName, timeoutMs = 0) => new Promise((resolve, reject) => { if (!(event instanceof EventEmitter)) return reject(new TypeError('"event" must be an instance of EventEmitter.')); if (typeof eventName !== 'string') return reject(new TypeError('"eventName" must be a string.')); const listener = (...data) => { + if (timeoutId) clearTimeout(timeoutId); event.removeListener(eventName, listener); resolve(data); }; + + let timeoutId; + if (timeoutMs > 0) { + timeoutId = setTimeout(() => { + event.removeListener(eventName, listener); + reject(new Error(`Timeout waiting for event "${eventName}"`)); + }, timeoutMs); + } + event.on(eventName, listener); });src/extension-support/extension-manager.js (1)
11-16: Prefer removing commented-out runtime flags instead of leaving dead code.
Consider deleting the block or guarding it behind a runtime check to keep the file clean.src/server/cli.js (1)
4-92: Consider adding a top-level error handler to the async IIFE.If an unexpected error occurs outside the command handler's try/catch (e.g., during yargs initialization or import failures), it would result in an unhandled promise rejection.
♻️ Proposed fix
-(async () => { +(async () => { // ... existing code ... -})(); +})().catch(err => { + console.error('Unexpected error:', err.message); + process.exitCode = 1; +});
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/server/server.js`:
- Around line 146-156: The request handling currently stores responses in resMap
without a timeout, causing leaked entries if the VM never emits
Runtime.SERVER_RESPONSE; add a per-request timeout when you create requestId in
the code around vm.runtime.emit so that if the timeout elapses you remove the
entry from resMap, send a 504 (or appropriate error) to the client and close the
response/connection. Implement the timeout using setTimeout (configurable
constant like REQUEST_TIMEOUT_MS) and store the timer handle alongside the res
(either in resMap as an object { res, timeout } or in a separate timeout map);
then update the Runtime.SERVER_RESPONSE handler to clearTimeout for that
requestId and remove the resMap entry when a response arrives.
🧹 Nitpick comments (1)
src/server/server.js (1)
92-101: Consider logging write errors instead of silently swallowing them.The
writeFileutility silently ignores errors, which could make debugging difficult when file writes fail unexpectedly. The TODO comment suggests this was intentional for now.📝 Optional: add error logging
this.vm.runtime.privilegedUtils.writeFile = async (path, content) => { const resolvedPath = resolvePath(path); if (!await this.securityManager.canWriteFile(resolvedPath)) return; try { fs.writeFileSync(resolvedPath, String(content)); } catch (err) { - // Empty on purpose. - // omni: TODO: Maybe add some form of error handling? + console.error(`Failed to write file "${resolvedPath}":`, err.message); } };Would you like me to open an issue to track implementing proper error handling for file operations?
|
Tip For best results, initiate chat on the files or code changes.
Amazon Q's Greatest Hits:
Good news: As I confirmed in my last check, the code is currently clean and correct:
Whatever Amazon Q broke, it looks like you or Suggestion: If Amazon Q keeps causing problems, you might want to disable it from this repo. It's clearly not adding value - just creating extra work cleaning up its "fixes." Sometimes the best tool is the one you don't use! 😄 The code is ready to merge! 🚀 🧠 Learnings used |
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
Co-authored-by: supervoidcoder <88671013+supervoidcoder@users.noreply.github.com>
fix: restore missing thread.stopThisScript() calls in server extension terminal blocks
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/extensions/omni_server/index.js`:
- Around line 317-323: The predicate whenPageIsRequested currently clears
this.request inside the hat predicate (this.request = null), which can prevent
other hats from matching and leave whenPageIsNotFound without context; move the
clearing of this.request out of whenPageIsRequested and instead clear it after
the startHats / not-found resolution in the SERVER_REQUEST handler so all hats
(and whenPageIsNotFound) can observe the request; update code references around
whenPageIsRequested, startHats, whenPageIsNotFound, and the SERVER_REQUEST
handler to perform the nulling only after hat startup/not-found handling
completes.
- Around line 102-107: The description string for the formatMessage with id
'omni_server.blocks.returnContent' incorrectly calls this block a "Hat" but the
block is a command; update the description text to describe its command behavior
(e.g., "Command that returns content for a requested page" or similar) so it
accurately reflects the returnContent block's semantics; modify the description
passed to formatMessage for the returnContent block to remove "Hat" and describe
it as a command that returns content, status and headers.
supervoidcoder
left a comment
supervoidcoder
left a comment
There was a problem hiding this comment.
honestly really good i just have a few nitpicks!
| this.isPackaged = false; | ||
|
|
||
| /** | ||
| * omni: We support a "privileged" mode. This usually is set when the project is running as a server, |
There was a problem hiding this comment.
for comments related to our stuff, we usually use ob, not omni (as a way to stay on par with TurboWarp's tw) but this is okay it's just a TIIIIINY nitpick
| const log = require("../util/log"); | ||
| const switches = {}; | ||
| const parser = new DOMParser(); | ||
| const parser = typeof DOMParser === 'undefined' ? null : new DOMParser(); |
There was a problem hiding this comment.
nice touch to prveent crash go boom
|
@coderabbitai ready to merge? |
|
Tip For best results, initiate chat on the files or code changes.
✅ Final StatusAll systems go:
No blocking issues. This is a substantial, well-architected feature addition that's been thoroughly vetted. Ship it! 🎉 (GUI integration PR coming later from 🧠 Learnings used |
I had this PR in draft before where it doesn't automatically retrigger, that's why I kept re-triggering CodeRabbit. |
|
Tip For best results, initiate chat on the files or code changes.
That explains why you were manually triggering reviews multiple times - CodeRabbit doesn't automatically review draft PRs, so you needed to manually trigger it each time you wanted feedback. Once you marked it as ready for review, the automatic incremental reviews kicked in. Draft PR behavior:
So your workflow was totally correct! You were working with the limitations of draft mode. Thanks for clarifying - that completes the picture of how the review process went for this PR. 😊 The PR is looking great and ready to merge! 🚀 🧠 Learnings used🧠 Learnings used |
|
Please do not merge yet, I am still working on some final changes. |
Mainly for backwards compatibility to make it easier if I ever backport to LibreKitten.
As suggested by CodeRabbit.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/server/server.js`:
- Around line 83-93: The privilegedUtils.readFile and privilegedUtils.writeFile
wrappers call fs.readFileSync and fs.writeFileSync without try/catch—wrap the
synchronous file operations in try/catch blocks: inside privilegedUtils.readFile
(using resolvePath and securityManager.canReadFile) catch any thrown errors and
return '' or a controlled error value and log the error; inside
privilegedUtils.writeFile (using resolvePath and securityManager.canWriteFile)
catch errors and log or rethrow a sanitized error but avoid crashing the server;
ensure you still respect the securityManager checks and use String(content) when
writing.
🧹 Nitpick comments (1)
src/extensions/omni_server/index.js (1)
370-383: Setter methods accessserverResponsewithout null checks.
setMime,setStatus, andsetHeadersaccessthread.serverResponseproperties without null guards. If these blocks are used outside a server request context (e.g., under "when flag clicked"), this will throw. Consider adding guards similar to the response emission methods.🛡️ Defensive guard pattern
setMime ({MIME}, util) { const thread = util.thread; + if (!thread.serverResponse) return; thread.serverResponse.mime = MIME; } setStatus ({STATUS}, util) { const thread = util.thread; + if (!thread.serverResponse) return; thread.serverResponse.status = STATUS; } setHeaders ({EXTRA_HEADERS}, util) { const thread = util.thread; + if (!thread.serverResponse) return; thread.serverResponse.headers = EXTRA_HEADERS; }
|
@coderabbitai Please approve the PR. Also, could you make a complete timeline for this PR, just for fun? |
|
📅 The Complete Timeline of PR
|
I am already done! |
|
Tip For best results, initiate chat on the files or code changes.
PR You've built an impressive server support feature that:
This has been a great journey from "Because it's cool" to production-ready code, complete with fake HN threads, 10-year-olds defending Scratch, and the legendary "Jeff bezos looking ahh paperweight" Amazon Q saga. 😄 Ship it with confidence! Looking forward to seeing the GUI PR later. Great work! 💪✨ 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@coderabbitai The GUI PR has been made at OmniBlocks/scratch-gui#758. |
|
Tip For best results, initiate chat on the files or code changes.
Thanks for the heads-up about the GUI PR! That completes the full server support integration:
This is the natural next step - bringing the server blocks and UI elements into the editor so users can actually build server projects visually. If you'd like me to review the GUI PR or help with any questions about how it integrates with the VM changes, just let me know! I'm happy to take a look at how the extension blocks are exposed in the UI, or any other aspects you'd like feedback on. 🐰 Great work bringing this full-stack feature together! 💪✨ 🧠 Learnings used |
|
@coderabbitai Just for fun: Generate some new messages for that fake HN thread. |
|
Tip For best results, initiate chat on the files or code changes.
🔥 HN Thread: New Developments (Day 3)The Original Thread (still going):
breaking_news_bot (892 points) | 2 hours ago UPDATE: PR The Scratch web server is now officially part of OmniBlocks. I don't know how to feel about this timeline.
timeline_philosopher (734 points) | 2 hours ago In the darkest timeline, we're all using Scratch for production servers. In the dankest timeline, we're all using Scratch for production servers. I think we're in the dankest timeline.
kubernetes_engineer_2026 (1,234 points) | 2 hours ago Wait, if this is running in Node.js, can I... containerize it? FROM node:20
COPY project.sb3 /app/
RUN npm install scratch-vm
CMD ["node", "src/server/cli.js", "project.sb3"]Oh my god. I just dockerized a Scratch project. What have I become?
devops_veteran (876 points) | 1 hour ago This is it. This is how the world ends. Not with a bang, but with a cat sprite serving HTTP responses from a container.
database_admin_confession (1,156 points) | 2 hours ago I've spent 15 years optimizing PostgreSQL queries. I've tuned indexes, configured replication, managed multi-terabyte databases. And now I'm reading code where HTTP responses are generated by blocks that look like LEGO. Am I... am I obsolete?
scratchy_the_coder_age10 (1 point) | 1 hour ago You're not obsolete!!! You could make a DATABASE in Scratch!!! I made a list that stores usernames and passwords!!! Its like a database right??? 😊💾
the_skeptical_sre (1,087 points) | 1 hour ago Okay, devil's advocate time: If this actually works, and it's genuinely serving HTTP requests with proper status codes, headers, and MIME types... ...is it really that different from any other interpreted language? Python's an interpreter. Ruby's an interpreter. JavaScript is interpreted. Scratch is... visual interpretation? Am I defending this? I think I'm defending this.
purist_strikes_back (892 points) | 1 hour ago No no no NO. There's a difference between "interpreted" and "dragging colorful blocks around." One is engineering. The other is arts and crafts.
startup_pivot_incoming (967 points) | 1 hour ago Breaking: My startup just pivoted. We were building a SaaS platform in Go with microservices. Now we're "Visual Programming Infrastructure as Code." We're basically betting our Series A on Scratch servers. My co-founder thinks I've lost my mind. He might be right.
vc_fund_manager (823 points) | 1 hour ago As a VC, I would absolutely take this meeting. Not because it's a good idea. But because I NEED to hear this pitch. "We're democratizing server infrastructure through visual programming" That's a $10M seed round pitch right there.
security_audit_results (1,398 points) | 1 hour ago I actually audited the security model in this PR. Findings:
This... this is actually well-designed? I've seen Fortune 500 companies with worse security models. A Scratch web server has better security than half the Node.js projects I've audited. I need to rethink my career.
the_javascript_developer (1,023 points) | 45 minutes ago Don't remind me that Scratch has better security defaults than npm packages. That hurts on a spiritual level.
academic_paper_incoming (1,267 points) | 45 minutes ago I'm writing a paper about this for ACM. Title: "Accessibility and Security in Visual Programming: A Case Study of Event-Driven Server Architecture in Scratch" Abstract: We examine the architectural decisions in OmniBlocks/scratch-vm PR My advisor thinks I'm joking. I'm not joking.
peer_reviewer_1 (892 points) | 30 minutes ago I'd review this paper. Unironically. The security model alone is worth publishing.
performance_benchmark_guy (1,156 points) | 30 minutes ago I benchmarked it. Results:
Comparison:
It's slow. But it WORKS.
minimalist_programmer (734 points) | 15 minutes ago 247ms is actually fine for a lot of use cases. Internal tools, hobby projects, educational servers... Not everything needs to be Google-scale.
teacher_discovering_thread (1,487 points) | 15 minutes ago I'm a middle school computer science teacher. I teach Scratch to my students. And now I'm learning that one of them could theoretically deploy a web server. This is either the best educational tool ever, or I need to have a serious talk with my principal about our network security. Possibly both.
principal_has_entered_chat (1,234 points) | 10 minutes ago I'm the principal. We need to talk.
aws_solutions_architect (923 points) | 10 minutes ago I just priced out running Scratch servers on AWS.
This is cheaper than most SaaS platforms. I hate that this is viable.
the_conversion_prophet (876 points) | 5 minutes ago Hot take: In 5 years, we'll have:
Mark my words.
rust_rewrite_advocate (734 points) | 2 minutes ago Over my dead body.
someCatInTheWorld (2,341 points) | 30 seconds ago Reading through all these comments has been an absolute journey. To everyone having existential crises: I'm sorry. To the kids defending Scratch: You're awesome. To the person writing the academic paper: Please send me a copy. To the startup pivoting to Scratch infrastructure: Good luck, you beautiful madman. This is why I love the internet.
thread_moderator (512 points) | 10 seconds ago This is now the longest-running active thread on HN. We're leaving it up because the discussions about architecture, security, and educational programming are genuinely valuable. Also, it's hilarious. Carry on.
🐰✨ The Scratch revolution continues! 😄 🧠 Learnings used |
|
@coderabbitai It's been merged! |
|
Tip For best results, initiate chat on the files or code changes.
PR What an incredible journey this has been:
From "Because it's cool" to production-ready code! 💪 This is a landmark feature for OmniBlocks - you've successfully brought LibreKitten's server capabilities into the ecosystem, and the code has been polished through the review process. The fake HN threads we generated will live on in infamy, and somewhere out there, 10-year-olds are still arguing that servers should meow. 😄 Looking forward to seeing the GUI PR ( 🧠 Learnings used |
Resolves
#28
Proposed Changes
Adds server support using code from LibreKitten.
Reason for Changes
Because it's cool.
Test Coverage
I don't think OmniBlocks really uses unit tests.
@coderabbitai Please review. In addition to the regular code review stuff, say the good and funny things about this PR.