diff --git a/static/panels.js b/static/panels.js index 8ab5cc14..f625103c 100644 --- a/static/panels.js +++ b/static/panels.js @@ -8520,7 +8520,7 @@ function _scheduleAppearanceAutosave(){ async function _autosaveAppearanceSettings(payload){ try{ - const saved=await api('/api/settings',{method:'POST',body:JSON.stringify(payload)}); + const saved=await _enqueueSettingsPost({method:'POST',body:JSON.stringify(payload)}); _settingsAppearanceAutosaveRetryPayload=null; _rememberAppearanceSaved(payload); if(saved&&saved.font_size){ @@ -8684,8 +8684,9 @@ function _preferencesPayloadFromUi(){ if(syncCb) payload.sync_to_insights=syncCb.checked; const updateCb=$('settingsCheckUpdates'); if(updateCb) payload.check_for_updates=updateCb.checked; - const updateChannelSel=$('settingsUpdateChannel'); - if(updateChannelSel) payload.update_channel=updateChannelSel.value; + // update_channel is NOT included here — it has its own dedicated write path + // (_saveUpdateChannelFromSelector) so a stale tab's generic autosave cannot + // overwrite a newer channel selection made in another tab. (#6612) const ignoreAgentUpdatesCb=$('settingsIgnoreAgentUpdates'); if(ignoreAgentUpdatesCb) payload.ignore_agent_updates=ignoreAgentUpdatesCb.checked; const whatsNewSummaryCb=$('settingsWhatsNewSummary'); @@ -8738,9 +8739,42 @@ function _speechPreferencesPayloadFromUi(){ return payload; } -function _setPreferencesAutosaveStatus(state){ +// Keep same-page settings merges FIFO so one response cannot race the next read-modify-write. +let _settingsPanelPostQueue=Promise.resolve(); + +function _enqueueSettingsPost(options){ + const requestOptions={...(options||{})}; + if(typeof requestOptions.body==='string') requestOptions.body=String(requestOptions.body); + const run=_settingsPanelPostQueue.then(async()=>{ + const saved=await api('/api/settings',requestOptions); + if(!saved||typeof saved!=='object'||Array.isArray(saved)){ + throw new Error('Invalid settings response'); + } + return saved; + }); + _settingsPanelPostQueue=run.catch(()=>{}); + return run; +} + +// Ownership token for the shared preferences autosave status slot. Prevents +// the channel writer from clearing or overwriting a 'failed'+Retry state the +// generic preferences autosave set; that Retry button has exactly one call +// site and becomes unreachable if another writer replaces the node. +let _preferencesAutosaveStatusOwner=null; + +function _setPreferencesAutosaveStatus(state,owner){ + owner=owner||'preferences'; const el=$('settingsPreferencesAutosaveStatus'); if(!el) return; + // Guard: if the slot shows 'failed' from a different writer, do not overwrite. + // The DOM class is the source of truth; an external clear would remove + // 'is-failed' and unblock writes without needing an extra variable. + if(_preferencesAutosaveStatusOwner&& + _preferencesAutosaveStatusOwner!==owner&& + el.classList.contains('is-failed')){ + return; + } + _preferencesAutosaveStatusOwner=state?owner:null; el.className='settings-autosave-status'; if(!state){ el.textContent=''; @@ -8782,7 +8816,7 @@ function _schedulePreferencesAutosave(){ async function _autosavePreferencesSettings(payload){ try{ - const saved=await api('/api/settings',{method:'POST',body:JSON.stringify(payload)}); + const saved=await _enqueueSettingsPost({method:'POST',body:JSON.stringify(payload)}); if(payload&&payload.terminal_auto_expand_on_output!==undefined){ window._terminalAutoExpandOnOutput=!!(saved&&saved.terminal_auto_expand_on_output); } @@ -8869,6 +8903,57 @@ function _retryPreferencesAutosave(){ _autosavePreferencesSettings(payload); } +let _channelSaveSeq=0; +// Last server-confirmed update_channel value. Seeded at panel hydration so the +// failure-revert path always has a known-good value. _confirmedUpdateChannel is +// the only reliable "previous" value: by the time a change event fires the +// browser has already applied the picked option to the before the change event fires), sync the badge to + match, and clear the status indicator without injecting a retry button. + + The stub models real listener semantics: _confirmedUpdateChannel='stable' + (what the server last confirmed), sel.value='experimental' (user's new pick, + already applied by the browser). After the failed POST the selector must + show 'stable' again and the badge must be synced to 'stable'. + """ + script = _channel_writer_script_prelude(PANELS_JS) + """ +(async () => { + const statusCalls = []; + const badgeCalls = []; + global._setPreferencesAutosaveStatus = function(s) { statusCalls.push(s); }; + global._syncUpdateChannelBadge = function(v) { badgeCalls.push(v); }; + global.api = async function() { throw new Error('network error'); }; + + // Simulate real browser behavior: server last confirmed 'stable', + // user picks 'experimental' -> browser applies it -> change event fires. + global._confirmedUpdateChannel = 'stable'; + const sel = { value: 'experimental' }; // already the new value + await _saveUpdateChannelFromSelector(sel); + console.log(JSON.stringify({ selectorValue: sel.value, statusCalls, badgeCalls })); +})().catch(err => { console.error(err.message); process.exit(1); }); +""" + result = json.loads(_run_node(script)) + assert result["selectorValue"] == "stable", ( + f"selector must revert to _confirmedUpdateChannel ('stable') on failure; " + f"got {result['selectorValue']!r}" + ) + assert result["badgeCalls"] == ["stable"], ( + f"badge must be synced to 'stable' on failure; got {result['badgeCalls']!r}" + ) + # Status sequence: 'saving' before POST, then null to clear on failure. + # Must NOT contain 'failed' (its retry button replays the wrong payload). + assert "saving" in result["statusCalls"], ( + f"'saving' status must be emitted before POST; got {result['statusCalls']!r}" + ) + assert "failed" not in result["statusCalls"], ( + f"'failed' must not be used (retry button replays wrong payload); " + f"got {result['statusCalls']!r}" + ) + # null clears the 'saving' indicator + assert None in result["statusCalls"], ( + f"null status must be emitted to clear the saving indicator on failure; " + f"got {result['statusCalls']!r}" + ) + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_channel_writer_undefined_response(): + """api() returning undefined on the 401 path must enter the failure branch.""" + script = _channel_writer_script_prelude(PANELS_JS) + """ +(async () => { + const statuses = []; + const badges = []; + global._setPreferencesAutosaveStatus = function(s) { statuses.push(s); }; + global._syncUpdateChannelBadge = function(v) { badges.push(v); }; + global.api = async function() { return undefined; }; + global._confirmedUpdateChannel = 'stable'; + const sel = { value: 'experimental' }; + await _saveUpdateChannelFromSelector(sel); + console.log(JSON.stringify({ selectorValue: sel.value, confirmed: _confirmedUpdateChannel, statuses, badges })); +})().catch(err => { console.error(err.message); process.exit(1); }); +""" + result = json.loads(_run_node(script)) + assert result["selectorValue"] == "stable", ( + f"undefined api() response must revert to the confirmed value 'stable'; " + f"got {result['selectorValue']!r}" + ) + assert result["confirmed"] == "stable", ( + f"_confirmedUpdateChannel must remain 'stable'; got {result['confirmed']!r}" + ) + assert result["badges"] == ["stable"] + assert result["statuses"] == ["saving", None] + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_channel_writer_string_response(): + """api() returning a string body on a non-JSON 200 must enter the failure branch.""" + script = _channel_writer_script_prelude(PANELS_JS) + """ +(async () => { + const statuses = []; + const badges = []; + global._setPreferencesAutosaveStatus = function(s) { statuses.push(s); }; + global._syncUpdateChannelBadge = function(v) { badges.push(v); }; + global.api = async function() { return 'login required'; }; + global._confirmedUpdateChannel = 'stable'; + const sel = { value: 'experimental' }; + await _saveUpdateChannelFromSelector(sel); + console.log(JSON.stringify({ selectorValue: sel.value, confirmed: _confirmedUpdateChannel, statuses, badges })); +})().catch(err => { console.error(err.message); process.exit(1); }); +""" + result = json.loads(_run_node(script)) + assert result["selectorValue"] == "stable", ( + f"string api() response must revert to the confirmed value 'stable'; " + f"got {result['selectorValue']!r}" + ) + assert result["confirmed"] == "stable", ( + f"_confirmedUpdateChannel must remain 'stable'; got {result['confirmed']!r}" + ) + assert result["badges"] == ["stable"] + assert result["statuses"] == ["saving", None] + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +@pytest.mark.parametrize("response", [None, [], 7, False], ids=["null", "array", "number", "boolean"]) +def test_channel_writer_non_object_response_reverts(response): + """All JSON non-object response shapes are failed saves, not confirmations.""" + script = _channel_writer_script_prelude(PANELS_JS) + f""" +(async () => {{ + const statuses = []; + global._setPreferencesAutosaveStatus = function(s) {{ statuses.push(s); }}; + global._syncUpdateChannelBadge = function() {{}}; + global.api = async function() {{ return {json.dumps(response)}; }}; + global._confirmedUpdateChannel = 'stable'; + const sel = {{ value: 'experimental' }}; + await _saveUpdateChannelFromSelector(sel); + console.log(JSON.stringify({{ selectorValue: sel.value, confirmed: _confirmedUpdateChannel, statuses }})); +}})().catch(err => {{ console.error(err.message); process.exit(1); }}); +""" + result = json.loads(_run_node(script)) + assert result == { + "selectorValue": "stable", + "confirmed": "stable", + "statuses": ["saving", None], + } + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_channel_writer_valid_object_missing_or_invalid_channel_falls_back_to_posted_value(): + """A valid JSON object with no usable channel preserves the server merge contract.""" + script = _channel_writer_script_prelude(PANELS_JS) + """ +(async () => { + const confirmed = []; + let response = {}; + global._setPreferencesAutosaveStatus = function() {}; + global._syncUpdateChannelBadge = function(v) { confirmed.push(v); }; + global.api = async function() { return response; }; + const missing = { value: 'experimental' }; + global._confirmedUpdateChannel = 'stable'; + await _saveUpdateChannelFromSelector(missing); + response = { update_channel: 'nightly' }; + const invalid = { value: 'experimental' }; + await _saveUpdateChannelFromSelector(invalid); + console.log(JSON.stringify({ missing: missing.value, invalid: invalid.value, confirmedValue: _confirmedUpdateChannel, confirmed })); +})().catch(err => { console.error(err.message); process.exit(1); }); +""" + result = json.loads(_run_node(script)) + assert result == { + "missing": "experimental", + "invalid": "experimental", + "confirmedValue": "experimental", + "confirmed": ["experimental", "experimental"], + } + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_status_slot_ownership(): + """Two writers share the single preferences autosave status slot. + + Scenario 1: generic autosave sets 'failed' with owner='preferences'. + Channel write succeeds; its 'saved' call must be blocked — the 'failed'+Retry + must stay visible so the user can retry the unsaved generic payload. + + Scenario 2: stale channel failure (superseded by a second write) fires a + null-clear via the seq guard. That null-clear must NOT fire because + seq!==_channelSaveSeq, so the generic autosave's 'failed' state is preserved. + """ + script = _node_prelude(PANELS_JS) + """ +global._preferencesAutosaveStatusOwner = null; + +// DOM stub: tracks className and textContent/innerHTML separately. +function makeStatusEl() { + return { + _cls: '', + _content: '', + get className() { return this._cls; }, + set className(v) { this._cls = v; }, + classList: null, // set below after object creation + set textContent(v) { this._content = v; }, + get textContent() { return this._content; }, + set innerHTML(v) { this._content = v; }, + }; +} +const statusEl = makeStatusEl(); +statusEl.classList = { + add(c) { statusEl._cls += (' ' + c); }, + contains(c) { return statusEl._cls.split(' ').includes(c); }, +}; +global.$ = function(id) { + if (id === 'settingsPreferencesAutosaveStatus') return statusEl; + return null; +}; +global.t = function(k) { return k; }; +global.esc = function(s) { return String(s); }; + +global._setPreferencesAutosaveStatus = (0, eval)( + '(' + extractFunc(panelsSrc, '_setPreferencesAutosaveStatus') + ')' +); +global._saveUpdateChannelFromSelector = (0, eval)( + '(' + extractFunc(panelsSrc, '_saveUpdateChannelFromSelector') + ')' +); +global._enqueueSettingsPost = (0, eval)( + '(' + extractFunc(panelsSrc, '_enqueueSettingsPost') + ')' +); + +(async () => { + const results = {}; + + // ── Scenario 1: generic autosave 'failed'; channel success must not overwrite ── + _setPreferencesAutosaveStatus('failed', 'preferences'); + results.s1_afterGenericFailed = { + cls: statusEl._cls.trim(), + owner: _preferencesAutosaveStatusOwner, + }; + + // Channel write fires and succeeds + global.api = async function() { return { update_channel: 'experimental' }; }; + global._confirmedUpdateChannel = 'stable'; + const sel1 = { value: 'experimental' }; + await _saveUpdateChannelFromSelector(sel1); + results.s1_afterChannelSuccess = { + cls: statusEl._cls.trim(), + owner: _preferencesAutosaveStatusOwner, + selectorValue: sel1.value, + }; + + // ── Scenario 2: stale channel failure's null-clear must not fire ── + // Reset state for a clean scenario. + statusEl._cls = ''; + statusEl._content = ''; + global._preferencesAutosaveStatusOwner = null; + global._channelSaveSeq = 0; + global._confirmedUpdateChannel = 'stable'; + + // Generic autosave sets 'failed'. + _setPreferencesAutosaveStatus('failed', 'preferences'); + results.s2_afterGenericFailed = { cls: statusEl._cls.trim() }; + + // Channel write 1 starts (seq=1) but blocks. + let resolveBlock; + const block = new Promise(r => { resolveBlock = r; }); + let callCount = 0; + global.api = async function(url, opts) { + callCount++; + if (callCount === 1) { await block; throw new Error('stale fail'); } + return { update_channel: 'stable' }; + }; + + const p1 = _saveUpdateChannelFromSelector({ value: 'experimental' }); // seq=1, blocks + + await Promise.resolve(); + // Channel write 2 is queued behind write 1; seq advances but api is not called. + const sel2 = { value: 'stable' }; + const p2 = _saveUpdateChannelFromSelector(sel2); // seq=2 + await Promise.resolve(); + results.s2_whileFirstBlocked = { cls: statusEl._cls.trim(), owner: _preferencesAutosaveStatusOwner, callCount }; + + // Unblock write 1; its catch fires but seq(1)!==_channelSaveSeq(2) -> no null-clear. + resolveBlock(); + await p1; + await p2; + results.s2_afterSecondWrite = { cls: statusEl._cls.trim(), owner: _preferencesAutosaveStatusOwner }; + results.s2_afterStaleFailure = { cls: statusEl._cls.trim(), owner: _preferencesAutosaveStatusOwner }; + + console.log(JSON.stringify(results)); +})().catch(err => { console.error(err.stack || err.message); process.exit(1); }); +""" + result = json.loads(_run_node(script)) + + # Scenario 1 + assert "is-failed" in result["s1_afterGenericFailed"]["cls"], ( + f"generic 'failed' must be set; got {result['s1_afterGenericFailed']!r}" + ) + assert result["s1_afterGenericFailed"]["owner"] == "preferences", ( + f"owner must be 'preferences'; got {result['s1_afterGenericFailed']['owner']!r}" + ) + # Channel write succeeded but must NOT overwrite 'failed' from 'preferences' + assert "is-failed" in result["s1_afterChannelSuccess"]["cls"], ( + f"channel 'saved' must be blocked by ownership guard; " + f"cls is now {result['s1_afterChannelSuccess']['cls']!r}" + ) + assert result["s1_afterChannelSuccess"]["selectorValue"] == "experimental", ( + f"selector must still update even when status write is blocked; " + f"got {result['s1_afterChannelSuccess']['selectorValue']!r}" + ) + + # Scenario 2 + assert "is-failed" in result["s2_afterGenericFailed"]["cls"], ( + f"generic 'failed' must be set before channel write; got {result['s2_afterGenericFailed']!r}" + ) + assert result["s2_whileFirstBlocked"]["callCount"] == 1, ( + f"queued write must not reach api before write 1 resolves; got {result['s2_whileFirstBlocked']!r}" + ) + # After write 2 succeeds: write 2 tried to write 'saving' then 'saved', but + # generic 'failed' blocks both → slot still shows 'failed' + assert "is-failed" in result["s2_afterSecondWrite"]["cls"], ( + f"generic 'failed' must survive write 2; got {result['s2_afterSecondWrite']!r}" + ) + # After stale write 1's failure: seq guard blocks the null-clear → slot unchanged + assert "is-failed" in result["s2_afterStaleFailure"]["cls"], ( + f"stale failure's null-clear must be blocked by seq guard; " + f"cls is now {result['s2_afterStaleFailure']['cls']!r}" + ) + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +def test_channel_writer_concurrent_selection(tmp_path, monkeypatch): + """Two rapid selections serialize server writes and retain the latest UI value.""" + script = _channel_writer_script_prelude(PANELS_JS) + """ +(async () => { + const results = []; + let resolveFirst; + const firstCallBlocker = new Promise(resolve => { resolveFirst = resolve; }); + let apiCallCount = 0; + const completed = []; + + global.api = async function(url, opts) { + apiCallCount++; + const body = JSON.parse(opts.body); + if (apiCallCount === 1) await firstCallBlocker; + completed.push(body); + return { update_channel: body.update_channel }; + }; + + const sel = { value: 'experimental' }; + global._confirmedUpdateChannel = null; + const p1 = _saveUpdateChannelFromSelector(sel); // seq=1, blocked + sel.value = 'stable'; // user changes selection again + const p2 = _saveUpdateChannelFromSelector(sel); // seq=2, queued + + await Promise.resolve(); + results.push({ while_first_blocked: sel.value, apiCallCount, completed }); + + resolveFirst(null); // unblock first call's response, then start the queued second call + await Promise.all([p1, p2]); + results.push({ after_both_complete: sel.value, apiCallCount, completed, confirmed: _confirmedUpdateChannel }); + + console.log(JSON.stringify({ results, finalSeq: _channelSaveSeq })); +})().catch(err => { console.error(err.message); process.exit(1); }); +""" + result = json.loads(_run_node(script)) + results = result["results"] + import api.config as config + + settings_file = tmp_path / "settings.json" + monkeypatch.setattr(config, "SETTINGS_FILE", settings_file) + config.save_settings({"update_channel": "stable"}) + for body in results[1]["completed"]: + config.save_settings(body) + persisted = config.load_settings().get("update_channel") + assert results[0]["while_first_blocked"] == "stable", ( + f"second selection must remain visible while first request is queued; got {results[0]!r}" + ) + assert results[0]["apiCallCount"] == 1, ( + f"only the first settings POST may be in flight; got {results[0]!r}" + ) + assert results[1]["after_both_complete"] == "stable", ( + f"after both serialized writes, selector must be 'stable'; " + f"got {results[1]!r}" + ) + assert results[1]["completed"] == [ + {"update_channel": "experimental"}, + {"update_channel": "stable"}, + ], results + assert persisted == "stable", results + assert results[1]["confirmed"] == "stable", results + assert results[1]["apiCallCount"] == 2, results + assert result["finalSeq"] == 2, ( + f"_channelSaveSeq must be 2 after two calls; got {result['finalSeq']!r}" + ) + diff --git a/tests/test_sprint33.py b/tests/test_sprint33.py index 0a4066ae..51776c82 100644 --- a/tests/test_sprint33.py +++ b/tests/test_sprint33.py @@ -94,7 +94,7 @@ def test_save_settings_password_change_preflights_current_password_before_api(): body = match.group(1) assert "currentPwField=$('settingsCurrentPassword')" in body assert "showToast(t('current_password_required'))" in body - assert body.index("showToast(t('current_password_required'))") < body.index("api('/api/settings'") + assert body.index("showToast(t('current_password_required'))") < body.index("_enqueueSettingsPost(") def test_disable_auth_typed_confirm_locales_show_literal_phrase():