Conversation
There was a problem hiding this comment.
Pull request overview
Refactors codeworld-server and the web client to remove legacy “program/deploy ID” plumbing and instead key locally-saved code by a content hash, while also trimming obsolete Haskell modules/dependencies and tightening imports/warnings.
Changes:
- Server
compileresponse no longer includes program/deploy IDs; client-side code now hashes source and persists it locally. - Removed legacy
Util/Modelmodules and pruned unusedcodeworld-serverdependencies. - Updated JS compile/run flows to consume the new server response format.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/js/run.js | Adjust response parsing to load compiled program from new response layout. |
| web/js/codeworld_shared.js | Adds sha256digest helper for hashing source code. |
| web/js/codeworld.js | Switches editor initialization + compile flow to localStorage-by-hash and new compile response parsing. |
| codeworld-server/src/Util.hs | Deleted legacy utility layer (IDs, hashing, filesystem helpers). |
| codeworld-server/src/Model.hs | Deleted legacy JSON models tied to removed functionality. |
| codeworld-server/src/Main.hs | Removes program/deploy ID handling from compile endpoint and simplifies temp build layout. |
| codeworld-server/codeworld-server.cabal | Removes now-obsolete dependencies and modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const parts = responseText.split('\n=======================\n') | ||
|
|
||
| let hash, dhash, msg; | ||
| if (status < 500) { | ||
| if (responseText.length === 23) { | ||
| // will not happen | ||
| hash = responseText; | ||
| dhash = null; | ||
| } else { | ||
| try { | ||
| hash = parts[0]; | ||
| dhash = parts[1]; | ||
| msg = parts[2]; | ||
| if(msg) { | ||
| msg = msg.replace(/^[\r\n]+|[\r\n]+$/g, ''); | ||
| } else { | ||
| msg = 'Sorry! Your program couldn\'t be run right now.'; | ||
| } | ||
| if(status < 500) { | ||
|
|
||
| window.program = parts[3]; | ||
| run(hash,dhash,msg,false,compileGeneration); | ||
| localStorage.setItem(`${window.buildMode}-${hash}`, window.codeworldEditor.getValue()); | ||
| } catch (e) { | ||
| hash = ''; | ||
| } | ||
| const compilerMessage = parts[0]; | ||
| let compiledProgram = parts[1]; | ||
|
|
||
| if(!compilerMessage) { | ||
| compilerMessage = 'Sorry! Your program couldn\'t be run right now.'; | ||
| } | ||
| } | ||
|
|
||
| if (!hash) { | ||
| sweetAlert({ | ||
| title: Alert.title('Could not compile'), | ||
| text: 'The compiler is unavailable. Please try again later.', | ||
| type: 'error', | ||
| }); | ||
| return; | ||
| } else { | ||
| const codeHash = await sha256digest(src.trim()); | ||
|
|
||
| window.program = compiledProgram; | ||
| run(codeHash,"deploy_hash",compilerMessage,false,compileGeneration); | ||
| localStorage.setItem(`${window.buildMode}-${codeHash}`, src); |
There was a problem hiding this comment.
This branch runs for any status < 500, including compile errors (400). In those cases parts[1] will be missing, yet the code still sets window.program and calls run(...), which can lead to trying to execute an undefined program and hides the real compile errors. Gate the “run program” path on status === 200 and handle non-200 responses by showing/parsing the error text and not attempting to run.
| sendHttp('POST', 'compile', data, (request) => { | ||
| const { status, responseText } = request; | ||
| if(status < 500) { | ||
| const parts = responseText.split('\n=======================\n'); | ||
| const program = parts[3]; | ||
| const program = parts[1]; |
There was a problem hiding this comment.
The callback injects parts[1] into a <script> for any status < 500, which includes 400 compile errors where the response only contains an error message. That makes program undefined (or the string "undefined") and can cause confusing runtime failures. Only inject/execute the script when status === 200 and a program part is present; otherwise surface the compile error text.
| mode <- getBuildMode | ||
| let previewConf = previewConfig $ config ctx | ||
| Just source <- (T.decodeUtf8 <$>) <$> getParam "source" | ||
| mPreview <- getParam "enablePreview" |
There was a problem hiding this comment.
These Just ... <- getParam ... bindings are failable patterns; with -Wall they trigger -Wincomplete-uni-patterns and at runtime will fail with a 500 if the parameter is missing. Prefer explicitly handling Nothing (e.g., return a 400 with a clear message) to both avoid warnings and make the handler robust.
| (_,Left err) <- runCompile ctx mode sourceWithHolePlaceholders | ||
|
|
||
| let holes = extractHolesFromErrorText error | ||
| let holes = extractHolesFromErrorText err | ||
| replacementMap = defaultHoleValues previewConf | ||
| Just withDefaultValues = replaceHolesWithDefaultValue holes replacementMap source |
There was a problem hiding this comment.
This preview path relies on failable pattern matches ((_, Left err) <- ... and Just withDefaultValues = ...). With warnings re-enabled these will also trigger incomplete-pattern warnings, and at runtime any unexpected compile output (or Nothing from replaceHolesWithDefaultValue) will crash the request. Refactor to explicit case/maybe handling and return a controlled error response when assumptions don’t hold.
| projectModuleFinder :: Maybe FilePath -> BuildMode -> String -> IO (Maybe FilePath) | ||
| projectModuleFinder mSourceDir mode modName | ||
| projectModuleFinder mSourceDir _ modName | ||
| | length modName /= 23 || '.' `elem` modName = return Nothing | ||
| | "P" `isPrefixOf` modName = go (ProgramId (T.pack modName)) | ||
| | "P" `isPrefixOf` modName = go | ||
| | otherwise = return Nothing |
There was a problem hiding this comment.
projectModuleFinder now returns program.hs for any imported module whose name looks like a legacy P... program id. Since there is no longer a corresponding per-id source file, this will resolve unrelated P... imports to the current program.hs, leading to misleading errors (or even recursive/self imports). If importing by program id is no longer supported, this branch should return Nothing instead of mapping to program.hs.
| const compilerMessage = parts[0]; | ||
| let compiledProgram = parts[1]; | ||
|
|
||
| if(!compilerMessage) { | ||
| compilerMessage = 'Sorry! Your program couldn\'t be run right now.'; | ||
| } |
There was a problem hiding this comment.
compilerMessage is declared as const but then reassigned in the if (!compilerMessage) block, which will throw a runtime error. Also, an empty compiler output is a valid successful case (no errors), so treating empty as “couldn't be run” will incorrectly show an error on successful compiles.
This removes old leftovers from the original
codeworld-serverimplementation.Mentionable changes:
incomplete-patterns,name-shadowing,unused-importsandunused-matcheswarnings