Merge pull request #498 from makermelissa-piclaw/fix/issue-460-save-run-await

Honor save failures and surface USB-MSC hint when the filesystem is locked

Author: makermelissa

js/common/web-file-transfer.js

@@ -50,9 +50,36 @@ class FileTransferClient {
         }
     }
 
+    // Build a ProtocolError-shaped error that callers can recognize as
+    // "the device's filesystem is currently held by something else"
+    // (typically USB MSC). Tagged identically to the runtime PUT 409/500
+    // path so `saveFileContents()` can show the same actionable dialog
+    // whether the check trips on the cached writable flag or on the
+    // actual response from the device.
+    _writeProtectedError() {
+        const err = new ProtocolError("File System is Read Only.");
+        err.status = 409;
+        err.writeProtected = true;
+        err.hint = "The board's filesystem is currently locked, " +
+                   "usually because CIRCUITPY is mounted on a " +
+                   "computer over USB. Disconnect the USB cable, " +
+                   "or disable USB Mass Storage in boot.py, then " +
+                   "reset the board and try saving again. " +
+                   "(Ejecting the drive in your OS may not be " +
+                   "enough on its own.)";
+        err.helpUrl = "https://learn.adafruit.com/getting-started-with-web-workflow-using-the-code-editor/device-setup#disabling-usb-mass-storage-3125964";
+        err.helpLabel = "Disabling USB Mass Storage (Adafruit Learn)";
+        return err;
+    }
+
     async _checkWritable() {
+        // Force a re-read of the writable flag so the user can recover
+        // without disconnecting: if they just released the drive (or
+        // disabled USB MSC and reset), the next save attempt should
+        // succeed, not bounce off a stale `false` cache.
+        this._writable = null;
         if (await this.readOnly()) {
-            throw new Error("File System is Read Only. Try disabling the USB Drive.");
+            throw this._writeProtectedError();
         }
     }
 
@@ -72,7 +99,8 @@ class FileTransferClient {
             options.headers['Content-Type'] = "application/octet-stream";
         }
 
-        await this._fetch(`/fs${path}`, options);
+        const response = await this._fetch(`/fs${path}`, options);
+        return response.ok;
     }
 
     // Makes the directory and any missing parents
@@ -120,7 +148,32 @@ class FileTransferClient {
         }
 
         if (!response.ok) {
-            throw new ProtocolError(response.statusText);
+            // Attach the status code + a friendly hint when we recognize
+            // the failure mode, so callers can branch on it (e.g. show an
+            // actionable message and skip retries that won't help).
+            const err = new ProtocolError(response.statusText || `HTTP ${response.status}`);
+            err.status = response.status;
+            err.method = (fetchOptions.method || "GET").toUpperCase();
+            err.path = location;
+            // /fs/ PUT against a write-protected filesystem currently returns
+            // 500 on shipped CircuitPython firmware. A fix is pending to
+            // return 409 Conflict (matching DELETE / MOVE / mkdir-PUT in
+            // the same file). Treat both the same way until enough users
+            // are on the patched firmware that 500 can be left generic.
+            const isFsWrite = err.method === "PUT" &&
+                              typeof location === "string" &&
+                              location.startsWith("/fs/");
+            if (isFsWrite && (response.status === 409 || response.status === 500)) {
+                // Reuse the same wording/hint as the cached-flag
+                // _checkWritable() path, so users see one consistent
+                // message regardless of which layer caught the lock.
+                const wp = this._writeProtectedError();
+                err.writeProtected = true;
+                err.hint = wp.hint;
+                err.helpUrl = wp.helpUrl;
+                err.helpLabel = wp.helpLabel;
+            }
+            throw err;
         }
 
         return response;

js/script.js

@@ -229,8 +229,11 @@ async function newFile() {
 
 async function saveRunFile() {
     if (await checkConnected()) {
+        // workflow.saveFile() now propagates the real save result -- only
+        // soft-restart / re-import once the PUT actually succeeded. Otherwise
+        // we would reboot the board running the old code.py while the editor
+        // still had the unsaved edits (issue #460).
         if (await workflow.saveFile()) {
-            setSaved(true);
             await workflow.runCurrentCode();
         }
     }
@@ -328,7 +331,14 @@ async function checkReadOnly() {
         await showMessage(readOnly);
         return false;
     } else if (readOnly) {
-        await showMessage("Warning: File System is in read only mode. Disable the USB drive to allow write access.");
+        // Concise connect-time notice that the filesystem is read-only,
+        // with a link to the Learn guide for users who want the fix now.
+        const learnUrl = "https://learn.adafruit.com/getting-started-with-web-workflow-using-the-code-editor/device-setup#disabling-usb-mass-storage-3125964";
+        await showMessage(
+            "Filesystem is read-only — you can browse files, but saving " +
+            "will fail until USB Mass Storage is released. " +
+            `<a href="${learnUrl}" target="_blank" rel="noopener noreferrer">How to fix</a>.`
+        );
     }
     return true;
 }
@@ -535,47 +545,110 @@ async function loadEditor() {
 }
 
 var editor;
-var currentTimeout = null;
-var saveRetryCount = 0;
 const MAX_SAVE_RETRIES = 3;
+const SAVE_RETRY_DELAY_MS = 2000;
+let saveInFlight = false;
 
-// Save the File Contents and update the UI
+function sleep(ms) {
+    return new Promise((resolve) => setTimeout(resolve, ms));
+}
+
+// Save the File Contents and update the UI. Returns true on success, false
+// on final failure (after all retries). Retries inline so callers (Save+Run,
+// hotkeys, dialogs) can actually await the outcome -- previously this used
+// a fire-and-forget setTimeout, which let Save+Run soft-restart the board
+// before the PUT had succeeded (issue #460).
 async function saveFileContents(path) {
-    // If this is a different file, we write everything
-    if (path !== workflow.currentFilename) {
-        unchanged = 0;
-    }
-    let doc = editor.state.doc;
-    let offset = 0;
-    let contents = doc.sliceString(0);
-    if (workflow.partialWrites) {
-        offset = unchanged;
-        console.log("sync starting at", unchanged, "to", editor.state.doc.length);
+    if (saveInFlight) {
+        // Re-entrant save (e.g. user mashing Ctrl-S / Save+Run). The first
+        // call will report success/failure; the second would race the same
+        // bytes onto the wire and confuse partialWrites bookkeeping.
+        console.log("saveFileContents: already in flight, ignoring re-entry");
+        return false;
     }
-    let oldUnchanged = unchanged;
-    unchanged = doc.length;
+    saveInFlight = true;
     try {
-        if (await workflow.writeFile(path, contents, offset)) {
-            setFilename(workflow.currentFilename);
-            setSaved(true);
-            saveRetryCount = 0;
-        } else {
-            await showMessage(`Saving file '${workflow.currentFilename}' failed.`);
-        }
-    } catch (e) {
-        console.error("write failed", e, e.stack);
-        unchanged = Math.min(oldUnchanged, unchanged);
-        if (currentTimeout != null) {
-            clearTimeout(currentTimeout);
+        // If this is a different file, we write everything
+        if (path !== workflow.currentFilename) {
+            unchanged = 0;
         }
-        saveRetryCount++;
-        if (saveRetryCount < MAX_SAVE_RETRIES) {
-            console.log(`Save retry ${saveRetryCount} of ${MAX_SAVE_RETRIES}...`);
-            currentTimeout = setTimeout(() => saveFileContents(path), 2000);
-        } else {
-            saveRetryCount = 0;
-            await showMessage(`Saving file '${workflow.currentFilename}' failed after multiple attempts. Check your connection and try again.`);
+        let doc = editor.state.doc;
+        let contents = doc.sliceString(0);
+        let baseUnchanged = unchanged;
+        let docLengthAtStart = doc.length;
+
+        for (let attempt = 1; attempt <= MAX_SAVE_RETRIES; attempt++) {
+            // Recompute offset each attempt -- if onTextChange fired between
+            // retries, `unchanged` may have shrunk and we need to resend more.
+            let offset = 0;
+            if (workflow.partialWrites) {
+                offset = Math.min(baseUnchanged, unchanged);
+                console.log("sync starting at", offset, "to", editor.state.doc.length);
+            }
+            // Optimistically mark the bytes-being-sent as unchanged. If the
+            // write throws we'll roll back to baseUnchanged for the next try.
+            unchanged = docLengthAtStart;
+            try {
+                if (await workflow.writeFile(path, contents, offset)) {
+                    setFilename(workflow.currentFilename);
+                    setSaved(true);
+                    return true;
+                }
+                // writeFile returned a falsy value without throwing -- treat
+                // as a soft failure and surface a message immediately.
+                await showMessage(`Saving file '${workflow.currentFilename}' failed.`);
+                setSaved(false);
+                return false;
+            } catch (e) {
+                console.error(`write failed (attempt ${attempt} of ${MAX_SAVE_RETRIES})`, e, e.stack);
+                unchanged = Math.min(baseUnchanged, unchanged);
+                // If the device cleanly told us the filesystem is held by
+                // someone else (most commonly USB-MSC: the host has
+                // CIRCUITPY mounted), retrying won't help -- surface an
+                // actionable hint immediately and bail. Older CircuitPython
+                // firmware returns 500 for this case, newer firmware
+                // returns 409 Conflict; web-file-transfer.js tags both
+                // with `writeProtected` so we can treat them the same way.
+                if (e && e.writeProtected) {
+                    setSaved(false);
+                    const learnUrl = e.helpUrl || "https://learn.adafruit.com/getting-started-with-web-workflow-using-the-code-editor/device-setup#disabling-usb-mass-storage-3125964";
+                    const learnLabel = e.helpLabel || "Disabling USB Mass Storage (Adafruit Learn)";
+                    // MessageModal renders via innerHTML, so real markup
+                    // (sections, list, link) is fine. Sections separate the
+                    // 'what happened', 'why', and 'how to fix' so users can
+                    // scan instead of parsing a wall of prose.
+                    await showMessage(
+                        `<p><strong>Could not save '${workflow.currentFilename}'.</strong></p>` +
+                        `<p>The board's filesystem is locked, usually because ` +
+                        `CIRCUITPY is mounted on a computer over USB.</p>` +
+                        `<p><strong>To fix:</strong></p>` +
+                        `<ul style="margin: 0.25em 0 0.5em 1.25em; padding: 0;">` +
+                            `<li>Disconnect the USB cable, <em>or</em></li>` +
+                            `<li>Disable USB Mass Storage in <code>boot.py</code>, then reset the board.</li>` +
+                        `</ul>` +
+                        `<p><em>Note:</em> ejecting the drive in your OS isn't always enough on its own.</p>` +
+                        `<p><a href="${learnUrl}" target="_blank" rel="noopener noreferrer">${learnLabel}</a></p>` +
+                        `<p>Your edits are still here — save again once the filesystem is writable.</p>`
+                    );
+                    return false;
+                }
+                if (attempt < MAX_SAVE_RETRIES) {
+                    await sleep(SAVE_RETRY_DELAY_MS);
+                    // Bail out if the user disconnected mid-retry.
+                    if (!workflow || !workflow.connectionStatus()) {
+                        setSaved(false);
+                        return false;
+                    }
+                }
+            }
         }
+        // All retries exhausted. Leave the editor marked dirty so the user
+        // knows the file on the board is still stale.
+        setSaved(false);
+        await showMessage(`Saving file '${workflow.currentFilename}' failed after multiple attempts. Check your connection and try again.`);
+        return false;
+    } finally {
+        saveInFlight = false;
     }
 }
 
@@ -611,19 +684,13 @@ async function onTextChange(update) {
         unchanged = 0;
     }
 
-    if (currentTimeout != null) {
-        clearTimeout(currentTimeout);
-    }
-
     setSaved(false);
 }
 
 function disconnectCallback() {
-    if (currentTimeout != null) {
-        clearTimeout(currentTimeout);
-        currentTimeout = null;
-    }
-    saveRetryCount = 0;
+    // saveInFlight is intentionally not forced here -- the in-flight
+    // saveFileContents loop checks connectionStatus() between retries and
+    // exits cleanly on its own, then clears the flag in its finally block.
     updateUIConnected(false);
 }
 

js/workflows/workflow.js

@@ -442,8 +442,14 @@ except ImportError:
         // canceled or rejected) is treated the same as null and does not get
         // forwarded to writeFile, where it would crash in _splitPath. See #327.
         if (path != null) {
-            await this._saveFileContents(path);
-            return true;
+            // Propagate the actual save result so Save+Run and other callers
+            // can avoid taking follow-up actions (soft-restart, import) when
+            // the underlying PUT failed. _saveFileContents returns false on
+            // exhausted retries; treating only an explicit `false` as failure
+            // keeps backwards compatibility with older saveFileFunc callbacks
+            // that returned undefined on success (issue #460).
+            const result = await this._saveFileContents(path);
+            return result !== false;
         }
         return false;
     }

sass/layout/_layout.scss

@@ -318,7 +318,32 @@
     display: none;
 
     &.prompt {
-        max-height: 365px;
+        max-height: 80vh;
+        // Flex column so the buttons stay pinned to the bottom and the
+        // content area can scroll when a dialog has rich/multi-section
+        // markup that overflows the modal height. Three-class selector
+        // beats `.popup-modal.is--visible` so the `display: flex`
+        // wins on specificity.
+        &.is--visible {
+            display: flex;
+            flex-direction: column;
+        }
+        #message {
+            overflow-y: auto;
+            flex: 1 1 auto;
+            min-height: 0;
+        }
+        .buttons {
+            flex: 0 0 auto;
+        }
+    }
+
+    // The message dialog often shows multi-section explanatory prose
+    // (e.g. the save-failure / read-only-filesystem help). Cap the
+    // width so paragraphs wrap at a comfortable reading measure rather
+    // than stretching the full viewport on a wide monitor.
+    &[data-popup-modal="message"] {
+        max-width: min(480px, 90vw);
     }
 
     &.shadow {