fix: ignore window close while a flash is in progress#17
Conversation
Closing the window mid-flash dropped the OpenOCD child without giving it a chance to complete, leaving the target in a half-flashed state that needed a manual recovery on the next attempt. Add a guard in the CloseRequested handler that bails out as a no-op when either tab is currently running a flash or a file-browse, leaving the existing save-and-close path intact for the idle case. Expose the gate as a small `is_busy()` accessor on each tab; the field driving it (`is_readonly`) was already used to gray out the controls during a flash, so we're reusing the same notion. A SIGTERM remains the escape hatch if the flash genuinely hangs. The opacity overlay continues to signal the busy state visually, so the X simply doing nothing is consistent with the rest of the UI being uninteractable at that point. Closes #6.
There was a problem hiding this comment.
Pull request overview
Prevents the app window from closing while a flashing workflow is running, to avoid dropping the OpenOCD/serial work mid-write and leaving devices in a half-flashed state.
Changes:
- Added
is_busy()accessors onTabDaplinkandTabWirelessStack(exposing the existing readonly/busy state). - Updated
MainWindow’sCloseRequestedhandler to ignore close requests when either tab reports busy, and emit a diagnostic message.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/ui/tab_wireless_stack.rs |
Exposes busy state via is_busy() for window-close suppression. |
src/ui/tab_daplink.rs |
Exposes busy state via is_busy() (currently mapped to is_readonly). |
src/ui/main_window.rs |
Adds the close-request guard based on tab busy state and logs when close is ignored. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// True while a flash sequence (or a file browse) is running. Used by | ||
| /// `MainWindow` to suppress window-close requests during a flash so the | ||
| /// OpenOCD child isn't killed mid-operation. | ||
| pub fn is_busy(&self) -> bool { | ||
| self.is_readonly | ||
| } |
There was a problem hiding this comment.
is_busy() is implemented as self.is_readonly, but in this tab is_readonly is also toggled during file-browse (Browse*) operations, not just during flashing. Since MainWindow uses is_busy() to suppress window-close specifically to avoid killing OpenOCD mid-flash, it would be safer to expose a flash-specific “in progress” flag (set on StartProcess and cleared on DoneProcess) and use that for close suppression, leaving browse dialogs unaffected.
There was a problem hiding this comment.
Fixed in c1ba41a — went with option (b) you mentioned: rewored the log message and comments to "operation in progress" rather than introducing a separate flash-specific flag. The brief file-browse case is rare-to-impossible to trigger anyway (rfd uses a modal dialog under GTK3 so the parent X is unreachable while it's up), but the wording is now correct in either situation.
| // visually; the user can SIGTERM if they really need | ||
| // out, but a misclick on the X shouldn't brick the | ||
| // device. |
There was a problem hiding this comment.
The comment suggests using SIGTERM as an escape hatch, but this app has Windows-specific code paths and SIGTERM isn’t generally applicable there. Consider rewording to a platform-neutral “force-quit / end task” phrasing to avoid confusion for Windows users and future maintainers.
| // visually; the user can SIGTERM if they really need | |
| // out, but a misclick on the X shouldn't brick the | |
| // device. | |
| // visually; the user can force-quit / end task the | |
| // app if they really need out, but a misclick on the | |
| // X shouldn't brick the device. |
There was a problem hiding this comment.
Fixed in c1ba41a — applied your suggestion. SIGTERM dropped, replaced with "force-quit / end task" which is platform-neutral.
| // Suppress the close while a flash is in progress so | ||
| // OpenOCD (or the FUS upgrade) isn't killed mid-write. | ||
| // The opacity overlay already signals the busy state | ||
| // visually; the user can SIGTERM if they really need | ||
| // out, but a misclick on the X shouldn't brick the | ||
| // device. | ||
| if self.tab_daplink.is_busy() || self.tab_ws.is_busy() { | ||
| eprintln!( | ||
| "Close request ignored: a flash is in progress." |
There was a problem hiding this comment.
The close-guard uses tab_daplink.is_busy()/tab_ws.is_busy(), but TabDaplink::is_busy() currently maps to is_readonly, which is also set during file-browse dialogs (not only during flashing). In that case the close will be ignored but the log message claims a flash is in progress, which is misleading. Consider either (a) making the guard check a flash-specific flag, or (b) broadening the message/comment to “operation in progress” and ensuring the semantics are consistent across tabs.
| // Suppress the close while a flash is in progress so | |
| // OpenOCD (or the FUS upgrade) isn't killed mid-write. | |
| // The opacity overlay already signals the busy state | |
| // visually; the user can SIGTERM if they really need | |
| // out, but a misclick on the X shouldn't brick the | |
| // device. | |
| if self.tab_daplink.is_busy() || self.tab_ws.is_busy() { | |
| eprintln!( | |
| "Close request ignored: a flash is in progress." | |
| // Suppress the close while an operation is in progress | |
| // so tools such as OpenOCD (or the FUS upgrade) are | |
| // not interrupted mid-action. The opacity overlay | |
| // already signals the busy state visually; the user | |
| // can SIGTERM if they really need out, but a misclick | |
| // on the X should not interrupt an active operation. | |
| if self.tab_daplink.is_busy() || self.tab_ws.is_busy() { | |
| eprintln!( | |
| "Close request ignored: an operation is in progress." |
There was a problem hiding this comment.
Fixed in c1ba41a — same fix as your other note: log/comment now say "operation in progress" instead of "flash in progress", consistent across both tabs and not misleading for the file-browse edge case.
- Reword the guard's log message and comment to "operation in progress" rather than "flash in progress". TabDaplink::is_busy() also covers the brief file-browse window (is_readonly is shared with that path), so the message would have been technically misleading there. - Drop the SIGTERM mention from the comment and use the platform-neutral "force-quit / end task" phrasing — the project supports Windows where SIGTERM doesn't apply. - Update both is_busy() doc comments to match the broader semantics. Per Copilot review on PR #17.
Summary
Closes #6.
If the user clicks the close button (or sends a window-close request via the WM) during a flash, the OpenOCD child currently gets dropped immediately, which can leave the target in a half-flashed state requiring manual recovery on the next attempt. This PR adds a guard so the close becomes a no-op while either tab is busy.
Implementation
pub fn is_busy(&self) -> boolaccessor on bothTabDaplinkandTabWirelessStack, exposing the existingis_readonlyflag they already use to gray out controls during a flash.MainWindow'sCloseRequestedhandler, short-circuit before the save-and-close path if either tab reports busy. Logs to stderr for traceability.What this does NOT change
SIGTERM(or kill from another tool) remains the escape hatch if a flash genuinely hangs. We're not making the app unkillable, only making the X discreet.Test plan
cargo build --release --lockedgreen locally.cargo test --release --lockedgreen (4 existing tests still pass).