Clean up / refactor codeworld-server#73
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.
| 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.
There was a problem hiding this comment.
This pattern is pretty much used for every route handler. The error message is indeed a bit misleading if we don't provide the required params ('No handler accepted "/compile" '). Should we change this?
There was a problem hiding this comment.
I'm willing to go with your judgement on this.
There was a problem hiding this comment.
One could argue that the routes in question are not meant to be hit by the users themselves (i.e. manually sending a request to the server) and thus we shouldn't need to care if the user understands what is wrong or not. But I would argue that it's still better to add at least add some more contextual information such that it's easier to debug for us. I'll update the handlers in just a moment.
|
Can be merged now, right? |
|
Yes |
This removes old leftovers from the original
codeworld-serverimplementation.Mentionable changes:
incomplete-patterns,name-shadowing,unused-importsandunused-matcheswarnings