Fix missing crash dialog for panics thrown inside requestAnimationFrame loop#3788
Fix missing crash dialog for panics thrown inside requestAnimationFrame loop#3788JustJ01 wants to merge 4 commits intoGraphiteEditor:masterfrom
Conversation
Summary of ChangesHello @JustJ01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical stability issue where the editor would become unresponsive instead of showing a crash dialog when a panic occurred within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of the editor freezing on panics within the requestAnimationFrame loop by introducing a non-blocking, multi-stage panic handling strategy. The approach of using a direct JS callback to bypass mutex contention on EDITOR_HANDLE is a solid solution. The code is well-structured and the changes align with the detailed explanation in the PR description. My review includes one critical comment to remove leftover test code and a couple of medium-severity suggestions for improving code quality and performance.
I am having trouble creating individual review comments. Click here to see my feedback.
editor/src/messages/portfolio/document/node_graph/node_properties.rs (1287)
This panic!() call appears to be for testing purposes to verify the crash dialog behavior. It should be removed before this pull request is merged to prevent intentional crashes in the application.
frontend/wasm/src/editor_api.rs (100-109)
There's significant code duplication between this function and the #[cfg(feature = "native")] version of create below. To improve maintainability and reduce redundancy, consider extracting the common logic for initializing the EditorHandle and PANIC_DIALOG_MESSAGE_CALLBACK into a separate helper function that both create functions can call.
frontend/wasm/src/lib.rs (84-90)
There's a redundant string allocation here. The panic_hook function already converts the panic info to a String. This function then receives it as a &str and converts it back to a String using to_string(), which is inefficient.
To avoid this extra allocation, you could change this function's signature to accept a &String and then use clone() to create the FrontendMessage. The call site in panic_hook would not need to change. The same improvement applies to the send_panic_dialog function.
fn send_panic_dialog_via_callback(panic_info: &String) -> bool {
let message = FrontendMessage::DisplayDialogPanic { panic_info: panic_info.clone() };
let message_type = message.to_discriminant().local_name();
let Ok(message_data) = serde_wasm_bindgen::to_value(&message) else {
log::error!("Failed to serialize crash dialog panic message");
return false;
};|
@JustJ01 the suggested allocation removal is bs. Calling |
@TrueDoctor I have made the changes |
Closes #3402
Panics triggered from code running in the recursive
requestedAnimationFramepath could freeze the editor without showing the crash dialog.This PR makes those panics follow the same crash-reporting flow as other panics.
The Root Cause:
The panic handler could clash while trying to lock
EDITOR_HANDLEduringrequestedAnimationFramedispatch, so the crash dialog message was not always sent in that path.Old panic code used a blocking lock (
lock()) , so panic handling could get stuck waiting for that same lock, as a result app froze.What I changed:
panic_hook()now uses non blocking strategy:send_panic_dialog(), usestry_lock(), not lock(),send_panic_dialog_deferred()withsetTimeout(0)and retry next tick.PANIC_DIALOG_MESSAGE_CALLBACK, that callback is set duringEditorHandle::create(), this path avoidsEDITOR_HANDLEmutex contention entirely, so it stills works duringrequestedAnimationFramerelated panic timing.Verification:
Add
panic!()inbrightness_contrast_properties()and select Brightness/Contrast node , Crash dialog now appears (no freeze)Screen.Recording.2026-02-19.011012.mp4