Skip to content

fix(settings): stop a failed settings write from saying it was saved - #736

Open
JDProfresh wants to merge 1 commit into
Ark0N:masterfrom
JDProfresh:fix/settings-save-failure-toast
Open

JDProfresh wants to merge 1 commit into
Ark0N:masterfrom
JDProfresh:fix/settings-save-failure-toast

Conversation

@JDProfresh

Copy link
Copy Markdown
Contributor

App Settings' save only recognises the two 403s it already handles, so any other failed PUT /api/settings (including the new 409 CONFLICT from #719) still ends with "Settings saved" and a closed modal.

Follow-up to #719, the App Settings part you said could come as its own PR.

Problem

With a settings.json that does not parse, the server now refuses the write with 409 CONFLICT and a message that says how to fix it. The browser never shows that message:

  • App Settings Save toasts "Settings saved" and closes the modal. The same happens for a 400, a 500, or a dropped connection.
  • Picking a run mode or saving a respawn preset fails with no word at all.
  • Stopping the tunnel from the welcome screen or the tunnel panel says "Tunnel stopped", although the server refused the write before it got to stopping the tunnel, so the tunnel is still running.

Cause

  • _apiPut answers a dropped connection with null and an HTTP error with a non-ok response; it never throws (api-client.js). saveAppSettings() checks the response only through _noteSettingsWriteRefusal (403 FORBIDDEN) and _handleTunnelEnableRefusal, then reaches showToast('Settings saved') (settings-ui.js:3419 on master) and closeAppSettings() (:3439).
  • _handleTunnelEnableRefusal is called for any failure while the tunnel switch is on (:3393), so a 409 with the switch on shows the bare 409 text as a tunnel error, turns the switch off on this device and closes the modal. A 500 with no JSON body shows "Tunnel refused: set CODEMAN_PASSWORD...".
  • The run mode (session-ui.js:742) and respawn preset (respawn-ui.js:609) writes only pass the response to _noteSettingsWriteRefusal.
  • The two tunnel stop paths toast "Tunnel stopped" without looking at the response (settings-ui.js:2309 and :2759).

What it does

  • App Settings: a failed settings PUT shows a warning, Saved on this device, not on the server: <reason>, where the reason is the server's error, else HTTP <status>, else the server could not be reached. The modal stays open. Nothing that assumes the server stored the save runs: Apply's refresh, the "Tunnel starting" toast, and the gesture-control reload. The toast stays until dismissed (duration: 0), since the 409 message is long and tells you what to do.
  • A second Save after a failed one still sends the plan-usage collection flip and still reloads for a gesture-control change. Both are worked out against what this device stored before, and the failed save had already stored the new values, so the retry would have skipped them. Until a save lands, they are compared against what the failed save started from.
  • The tunnel switch is treated as a tunnel refusal only on a 403, which is the only status the server uses for it. When you accept the no-password prompt, the retry now resends the whole save instead of only tunnelEnabled. Before, every other setting stayed unsaved while the toast said "Settings saved".
  • Run mode and respawn presets: Run mode saved on this device, not on the server: <reason> / Respawn presets saved on this device, not on the server: <reason>. They stay saved on this device, as before.
  • Tunnel stop: Tunnel not stopped: <reason>, and the welcome button stays on.
  • The admin-only 403 and the tunnel's no-password 403 work as before. zh-CN entries for the new strings (the server's reason stays as it came).

Two small helpers in settings-ui.js next to _noteSettingsWriteRefusal: _settingsWriteError(res) returns the reason or '', and _noteSettingsWriteResult(res, what) is what the run mode and preset writes now call.

Two cases I left out:

  • The respawn preset migration at page load (settings-ui.js:5098 on master) and saveLastUsedCase (session-ui.js:644). Neither tells the user something was saved: the migration runs again on the next load, and quick-start already writes lastUsedCase on the server.
  • saveModelConfigFromSettings still ignores its own response. It writes the same file through the same queue right after the settings PUT, so when the file is broken the settings PUT fails first and that warning shows. Making the model config fail on its own would need a schema 400 the form can't produce, or the file getting torn between two back-to-back writes.

Tests

test/settings-save-failure-ui.test.ts runs the real settings-ui.js, session-ui.js, respawn-ui.js and api-client.js against the real index.html form; only fetch is mocked. It covers 409, 400, 500 with no JSON body, and a dropped connection for the save. It also covers Apply, the tunnel switch on, the gesture reload, a failed Save followed by a successful one, the accepted no-password retry (Save and Apply), run mode and presets, both tunnel stop paths, and the zh-CN strings. Negative cases: a successful save, the 403 FORBIDDEN save, the no-password 403 cancelled, a successful background write, a FORBIDDEN background write, and a successful tunnel stop.

On master, 18 of its 24 tests fail; the 6 negative cases pass. I removed each guard in turn, and every removal turns at least one test red. test/settings-admin-only-ui.test.ts now runs _settingsWriteError for real instead of stubbing it, which catches a known non-admin's save (which sends nothing) being read as a dropped connection. test/plan-usage-collection-flip.test.ts pins the flip's argument by source text, so its pattern now names _prevServer.

Gate: typecheck, lint, format:check, check:frontend-syntax, check:browser-excludes and npm test pass, except test/tui/tui-switch-guard.test.ts, which also fails on master on this machine (tmux 3.2a).

Verified

On a throwaway instance with a torn settings.json, in Chromium:

Action Server master this branch
App Settings, Save 409 "Settings saved" (success), modal closed "Saved on this device, not on the server: ...settings.json is not a valid JSON object, so this change was not saved..." (warning), modal open
Run mode picked 409 no toast "Run mode saved on this device, not on the server: ..." (warning)
Tunnel panel, stop 409 "Tunnel stopped" "Tunnel not stopped: ..." (error)
App Settings, Save (file fixed) 200 "Settings saved", modal closed "Settings saved", modal closed

On this branch settings.json was left untouched by every failed write.

App Settings' save handled only the multi-user admin-only 403 and the tunnel's no-password 403, so a 409 for a settings.json that does not parse, a 400, a 500 or a dropped connection (`_apiPut` answers null) still toasted "Settings saved" and closed the modal, and the run mode and respawn preset writes failed with no word at all. Those failures now warn with the server's reason (else the HTTP status, else that the server could not be reached), the modal stays open, and nothing that assumes the server stored the save runs: Apply's refresh, the tunnel-starting toast and the gesture-control reload. Since the modal now invites a second Save, the plan-usage collection flip and the gesture reload are judged against the settings the failed save started from until a save lands, so the retry still sends the flip and still reloads.

A save with the tunnel switch on is read as a tunnel refusal only on a 403, so another failure no longer turns the switch off or shows the no-password message, and an accepted no-password retry now resends the whole save instead of the tunnel keys alone, which had left every other setting unsaved behind "Settings saved". Run mode and respawn presets stay saved on this device as before and now say they were not saved on the server. Stopping the tunnel from the welcome screen or the tunnel panel no longer says "Tunnel stopped" when the server refused the write, since the tunnel is then still running.
@Ark0N

Ark0N commented Oct 28, 2026

Copy link
Copy Markdown
Owner

Thank you @JDProfresh, this is a careful follow-up to #719, and the write-up made it quick to review. The PR makes every browser write to settings.json that the server did not store say so: App Settings Save and Apply keep the modal open with the server's reason, the run mode and respawn preset writes warn, a failed tunnel stop no longer reports "Tunnel stopped", and an accepted no-password retry now resends the whole save. I ran your new test against master's four frontend files and got the same 18 of 24 failures you describe. With the change all 24 pass, as do the touched settings and tunnel tests, typecheck, lint, format and the frontend checks.

One small thing I will apply at merge:

  1. The baseline is not recorded when a tunnel enable is abandoned (src/web/public/settings-ui.js:3388 and :3419). Both _abandonTunnelEnable() returns come after saveAppSettingsToStorage(settings) has written the new values to this device, but before _unsavedSettingsBaseline is set at :3432. So after the user cancels the no-password prompt (or the 403 fallback prompt), the next Save sends no showPlanUsageLimits flip and does not reload for a gesture-control change. I reproduced both in your harness. Master behaves the same, and your _prevServer baseline is what closes it: this._unsavedSettingsBaseline = _prevServer; before each of the two _abandonTunnelEnable(settings, keepOpen) calls. With that, both cases send showPlanUsageLimits: false and reload, and your 24 tests stay green.

Two notes that need nothing from you:

  • When the accepted no-password retry itself fails (a 409, for example), _handleTunnelEnableRefusal (settings-ui.js:2705-2722) still handles it as a tunnel failure: a normal-duration error toast, the switch turned off on this device, the modal closed. Now that the retry carries the whole save, that is really a save failure. It needs no login plus an unreadable /api/tunnel/status and a failing retry, and master does the same, so it can stay as it is.
  • A tunnel stop refused as admin-only (a multi-user role that had not resolved yet) shows Tunnel not stopped: <the 403 text> instead of the admin-only toast. That is already better than the old "Tunnel stopped".

I will merge with the two-line change above on top. Thank you again for following this one through every write path.

@JDProfresh

Copy link
Copy Markdown
Contributor Author

Thanks for the careful read, and for reproducing the abandoned-tunnel baseline in the harness. Setting the baseline to _prevServer before both _abandonTunnelEnable calls is the right fix, and I'm happy for it to go in at merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants