Desktop: Make shutdown more robust and fix panic caused by invalid viewport scale#3783
Desktop: Make shutdown more robust and fix panic caused by invalid viewport scale#3783timon-schelling wants to merge 2 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses a panic caused by an invalid viewport scale (NaN). The changes introduce checks in both the Rust backend and the TypeScript frontend to validate the scale value. While the fix is correct for the reported issue, my review includes suggestions to make these checks more robust by also handling non-finite values like Infinity, which could cause similar problems. I've recommended using is_finite() in Rust and Number.isFinite() in TypeScript for more comprehensive validation.
| match message { | ||
| ViewportMessage::Update { x, y, width, height, scale } => { | ||
| assert_ne!(scale, 0., "Viewport scale cannot be zero"); | ||
| assert!(scale > 0., "Viewport scale must be greater than zero"); |
There was a problem hiding this comment.
While this change correctly prevents NaN scale values which caused the original panic, it still allows Infinity. An infinite scale could be passed from the frontend and lead to other calculations resulting in Infinity or NaN, potentially causing other issues. Using is_finite() would make this assertion more robust.
| assert!(scale > 0., "Viewport scale must be greater than zero"); | |
| assert!(scale.is_finite() && scale > 0., "Viewport scale must be finite and greater than zero"); |
| if (!scale || scale <= 0) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The check !scale works for NaN and 0, but using Number.isFinite() is more explicit and robust. It will correctly handle Infinity, -Infinity, and NaN, which can occur if logicalWidth is zero. This ensures that only valid, finite, positive scale values are sent to the backend.
| if (!scale || scale <= 0) { | |
| continue; | |
| } | |
| if (!Number.isFinite(scale) || scale <= 0) { | |
| continue; | |
| } |
fixes