feat: compile error overlay#78
Conversation
|
Resolve conflicts before making it ready for review |
2612e8d to
a949147
Compare
a949147 to
ddabc9a
Compare
|
ready for review @hulxv |
Hi @shruti2522 sorry for the late reply. we're working on it via #68 and we're investigating some problems in MetaCall's Windows and MacOS so don't worry. |
There was a problem hiding this comment.
Pull request overview
Implements a browser compile-error overlay for dev mode by capturing build failures, broadcasting them over live-reload WebSocket, and persisting the last error across reloads.
Changes:
- Added
BuildErrorrebuild message +last_errorscaching to forward build failures to the browser. - Implemented an in-browser overlay in the live-reload script to render build errors.
- Added an initial build pass + ANSI-stripping to surface pre-existing compile errors more cleanly.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| metassr-cli/src/cli/dev.rs | Performs an initial build to capture errors early and logs errors with ANSI cleanup |
| metassr-cli/Cargo.toml | Adds dependencies needed for initial error capture/cleanup (metassr-bundler, regex) |
| crates/metassr-server/src/scripts/live-reload.js | Renders compile-error overlay and reacts to new build_error WS messages |
| crates/metassr-server/src/rebuilder.rs | Introduces BuildError events and caches last errors for overlay persistence |
| crates/metassr-server/src/live_reload.rs | Sends cached errors immediately on WS connect and includes errors in WS payload |
| crates/metassr-server/src/lib.rs | Wires last_errors into the live reload server construction |
| crates/metassr-server/Cargo.toml | Adds regex dependency used for ANSI stripping |
| crates/metassr-bundler/src/lib.rs | Captures bundler errors for consumption by dev server initial build path |
| crates/metassr-bundler/src/bundle.js | Normalizes bundler errors into resolved values so Rust can capture messages |
| TODO.md | Marks the compile error overlay task as complete |
| Cargo.toml | Adds workspace-level regex dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| out_dir: PathBuf, | ||
| building_type: BuildingType, | ||
| is_rebuilding: Arc<AtomicBool>, | ||
| pub last_errors: Arc<std::sync::Mutex<Option<Vec<String>>>>, |
There was a problem hiding this comment.
Making last_errors a public field exposes internal synchronization details (a Mutex<Option<Vec<String>>>) to consumers and makes it harder to evolve this type safely. Prefer keeping the field private and adding a small API (e.g., last_errors() returning an Arc<Mutex<...>>, or better: get_last_errors()/set_last_errors()/clear_last_errors() methods) so other modules don’t directly lock/mutate internals.
There was a problem hiding this comment.
@shruti2522 what do you think? any arguments to make for using pub here?
There was a problem hiding this comment.
It was the quickest way to expose the field to LiveReloadServer and dev.rs at the time, replaced it with proper accessor methods as suggested
| let rebuilder = Arc::clone(&rebuilder); | ||
|
|
||
| async move { | ||
| let ansi_regex = regex::Regex::new(r"\\u\{1b\}\[[0-9;]*m").unwrap(); |
There was a problem hiding this comment.
The ANSI-stripping regex is compiled multiple times (and the pattern only targets the literal text \\u{1b} form, not the common raw ESC byte \\x1b). Consider centralizing ANSI stripping into a single helper (compile once) and using a more robust approach (e.g., a crate like strip-ansi-escapes, or a regex that handles both ESC and escaped forms) so logs/overlay consistently display clean messages.
| format!("Client build failed: {}", e) | ||
| }; | ||
| // Clean up the error message for the terminal | ||
| let ansi_regex = Regex::new(r"\\u\{1b\}\[[0-9;]*m").unwrap(); |
There was a problem hiding this comment.
The ANSI-stripping regex is compiled multiple times (and the pattern only targets the literal text \\u{1b} form, not the common raw ESC byte \\x1b). Consider centralizing ANSI stripping into a single helper (compile once) and using a more robust approach (e.g., a crate like strip-ansi-escapes, or a regex that handles both ESC and escaped forms) so logs/overlay consistently display clean messages.
| let stype = self.rebuilder.building_type(); | ||
| if let Err(e) = ServerSideBuilder::new("", &out_dir, stype)?.build() { | ||
| let err_msg = format!("Server build failed: {}", e); | ||
| let ansi_regex = Regex::new(r"\\u\{1b\}\[[0-9;]*m").unwrap(); |
There was a problem hiding this comment.
The ANSI-stripping regex is compiled multiple times (and the pattern only targets the literal text \\u{1b} form, not the common raw ESC byte \\x1b). Consider centralizing ANSI stripping into a single helper (compile once) and using a more robust approach (e.g., a crate like strip-ansi-escapes, or a regex that handles both ESC and escaped forms) so logs/overlay consistently display clean messages.
| // remove existing overlay on backdrop click | ||
| overlay.addEventListener('click', (e) => { | ||
| if (e.target === overlay) dismissErrorOverlay(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The hint says the overlay “can only be dismissed by fixing the error”, but the UI currently allows dismissing via backdrop click and the close button. Either remove/disable the dismiss behaviors to match the message, or update the hint text to reflect that users can manually dismiss the overlay.
| // remove existing overlay on backdrop click | |
| overlay.addEventListener('click', (e) => { | |
| if (e.target === overlay) dismissErrorOverlay(); | |
| }); |
| }); | ||
|
|
||
| const hint = document.createElement('p'); | ||
| hint.textContent = 'This error occurred during the build process and can only be dismissed by fixing the error.'; |
There was a problem hiding this comment.
The hint says the overlay “can only be dismissed by fixing the error”, but the UI currently allows dismissing via backdrop click and the close button. Either remove/disable the dismiss behaviors to match the message, or update the hint text to reflect that users can manually dismiss the overlay.
| hint.textContent = 'This error occurred during the build process and can only be dismissed by fixing the error.'; | |
| hint.textContent = 'This error occurred during the build process. Fix the error to make this overlay disappear automatically, or close it manually to continue.'; |
| // Add a tiny delay to ensure the browser has attached the onmessage handler | ||
| tokio::time::sleep(std::time::Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Introducing a fixed 100ms delay adds latency to showing the overlay on reload and is generally brittle. Since the client assigns onmessage synchronously right after creating the WebSocket, you can likely remove this sleep; if ordering is still a concern, a more robust pattern is an explicit client “ready” message before the server sends cached errors.
| // Add a tiny delay to ensure the browser has attached the onmessage handler | |
| tokio::time::sleep(std::time::Duration::from_millis(100)).await; |
| let msg = res_str.replace("String(\"", "").replace("\")", ""); | ||
| let msg = msg.replace("\\n", "\n"); | ||
| *BUNDLING_ERROR.lock().unwrap() = Some(msg); |
There was a problem hiding this comment.
Bundling error detection/parsing currently depends on format!(\"{:?}\", result) and string surgery (e.g., stripping String(\"...\")), which is fragile and couples behavior to debug formatting. Prefer extracting the returned JS value as a real string/typed value (or returning Result from the bundling call) and propagating the error via the function return rather than a global BUNDLING_ERROR side channel.
| let msg = res_str.replace("String(\"", "").replace("\")", ""); | |
| let msg = msg.replace("\\n", "\n"); | |
| *BUNDLING_ERROR.lock().unwrap() = Some(msg); | |
| *BUNDLING_ERROR.lock().unwrap() = Some(res_str); |
| let mut err_guard = BUNDLING_ERROR.lock().unwrap(); | ||
| if let Some(err_msg) = err_guard.take() { | ||
| return Err(anyhow::anyhow!("{}", err_msg)); | ||
| } |
There was a problem hiding this comment.
Bundling error detection/parsing currently depends on format!(\"{:?}\", result) and string surgery (e.g., stripping String(\"...\")), which is fragile and couples behavior to debug formatting. Prefer extracting the returned JS value as a real string/typed value (or returning Result from the bundling call) and propagating the error via the function return rather than a global BUNDLING_ERROR side channel.
| // strip ANSI escape codes from terminal output including Rust escaped format like \u{1b}[31m | ||
| const cleanMessage = (msg || '').replace(/(\\u(?:\{1b}|001b)|[\u001b\u009b])[[\]()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g, ''); | ||
| pre.textContent = cleanMessage.replace('Client-side build failed: Bundling failed: "ERROR: Compilation errors:\n', '').replace('"\nFix the error and save the file — this overlay will disappear automatically.', ''); |
There was a problem hiding this comment.
The overlay output strips very specific hard-coded prefixes/suffixes from the error string. This is brittle (small formatting changes will break it) and risks removing legitimate content that happens to match those substrings. Consider sending structured fields over WS (e.g., { stage: 'client', message: '...' }) or just render cleanMessage as-is and format via UI components rather than string slicing.
|
Hi @fahdfady, any specific changes you would like me to make, I have resolved the previous reviews |
Compile error overlay implementation in browser (like Next.js/Vite)
changes made:
Vecin error state to capture all error messages like next jsbrowser overlay: