Stage-394 follow-up: profile-switch reconciliation + a11y switch role + server-side chat/settings filter
Per deep-review verdict SHIP-WITH-FIXES on PR #2636: 1. Profile-switch reconciliation: _refreshProfileSwitchBackground now re-fetches /api/settings and re-applies hidden_tabs for the new profile. Without this, Profile A's hidden-tabs choice stayed in effect under Profile B until the user opened Settings → Appearance. 2. A11y: switched chips from role=button + aria-pressed to role=switch + aria-checked. The pressed/not-pressed wording confused screen-reader users because chip-off looks like the off state. Added role=group + aria-labelledby on the container, and a :focus-visible style on the chips. 3. Server-side belt-and-suspenders: api/config.py now strips 'chat' and 'settings' from hidden_tabs at validation time, matching the client's apply- time filter. A tampered POST can no longer persist the forbidden values. 3 new regression tests added (chat/settings rejection, profile-switch wiring, chip a11y attributes). Co-authored-by: FrancescoFarinola <francesco.farinola@example.com>
This commit is contained in:
1 parent
5491a54285
commit
38933b288d
5 files changed
+88
-6
No files matched your search
+9
-2
@@ -4515,11 +4515,18 @@ def save_settings(settings: dict) -> dict:
|
||||
not isinstance(v, str) or not _SETTINGS_LANG_RE.match(v)
|
||||
):
|
||||
continue
|
||||
# Validate hidden_tabs: must be a list of non-empty strings
|
||||
# Validate hidden_tabs: must be a list of non-empty strings.
|
||||
# Belt-and-suspenders strip of "chat" and "settings" so a
|
||||
# malicious POST cannot lock the user out of the always-visible
|
||||
# nav tabs even though the client also filters them at apply time.
|
||||
# Stage-394 follow-up to #2636 deep review.
|
||||
if k == "hidden_tabs":
|
||||
if not isinstance(v, list):
|
||||
continue
|
||||
v = [s for s in v if isinstance(s, str) and s.strip()]
|
||||
v = [
|
||||
s for s in v
|
||||
if isinstance(s, str) and s.strip() and s not in {"chat", "settings"}
|
||||
]
|
||||
# Coerce bool keys
|
||||
if k in _SETTINGS_BOOL_KEYS:
|
||||
v = bool(v)
|
||||
|
||||
+2
-2
@@ -977,8 +977,8 @@
|
||||
<div style="font-size:11px;color:var(--muted);margin-top:4px" data-i18n="settings_desc_session_endless_scroll">When enabled, older messages load automatically as you scroll upward. When disabled, use the older-messages button.</div>
|
||||
</div>
|
||||
<div class="settings-field">
|
||||
<label data-i18n="settings_label_tab_visibility">Sidebar tabs</label>
|
||||
<div id="tabVisibilityChips" class="tab-visibility-chips"></div>
|
||||
<label id="tabVisibilityLabel" data-i18n="settings_label_tab_visibility">Sidebar tabs</label>
|
||||
<div id="tabVisibilityChips" class="tab-visibility-chips" role="group" aria-labelledby="tabVisibilityLabel"></div>
|
||||
<div style="font-size:11px;color:var(--muted);margin-top:4px" data-i18n="settings_desc_tab_visibility">Choose which tabs appear in the sidebar and rail. Chat and Settings are always visible.</div>
|
||||
</div>
|
||||
<div id="settingsAppearanceAutosaveStatus" class="settings-autosave-status" aria-live="polite"></div>
|
||||
|
||||
+17
-1
@@ -4508,6 +4508,17 @@ function _refreshProfileSwitchBackground(gen){
|
||||
if (gen !== _profileSwitchGeneration) return;
|
||||
if (S.session && typeof syncTopbar === 'function') syncTopbar();
|
||||
}).catch(()=>{});
|
||||
// Reconcile per-profile sidebar tab visibility. hidden_tabs is a per-profile
|
||||
// appearance setting; without this fetch, Profile A's hidden-tabs choice
|
||||
// would remain in effect under Profile B until the user opens Settings.
|
||||
// Stage-394 follow-up to #2636 deep review.
|
||||
Promise.resolve(api('/api/settings')).then(function(s){
|
||||
if (gen !== _profileSwitchGeneration) return;
|
||||
var hidden = (s && Array.isArray(s.hidden_tabs)) ? s.hidden_tabs : [];
|
||||
hidden = hidden.filter(function(x){ return typeof x === 'string' && x.trim(); });
|
||||
if (typeof _setHiddenTabs === 'function') _setHiddenTabs(hidden);
|
||||
if (typeof _applyTabVisibility === 'function') _applyTabVisibility(hidden);
|
||||
}).catch(function(){});
|
||||
}
|
||||
|
||||
async function loadProfilesPanel() {
|
||||
@@ -5167,7 +5178,12 @@ function _renderTabVisibilityChips(){
|
||||
if(isOff)chip.classList.add('chip-off');
|
||||
chip.textContent=label;
|
||||
chip.setAttribute('data-tab-panel',panel);
|
||||
chip.setAttribute('aria-pressed',isOff?'false':'true');
|
||||
// Use role="switch" + aria-checked instead of aria-pressed so screen
|
||||
// readers narrate "Tasks switch on/off" (matches user mental model) rather
|
||||
// than "Tasks toggle button pressed/not-pressed" (where the polarity is
|
||||
// confusing because chip-off looks like the "off" state).
|
||||
chip.setAttribute('role','switch');
|
||||
chip.setAttribute('aria-checked',isOff?'false':'true');
|
||||
chip.onclick=function(){_toggleTabVisibilityChip(panel);};
|
||||
container.appendChild(chip);
|
||||
});
|
||||
|
||||
@@ -2781,6 +2781,7 @@ main.main.showing-logs > #mainLogs{display:flex;}
|
||||
.tab-visibility-chips{display:flex;flex-wrap:wrap;gap:6px;margin-top:4px;}
|
||||
.tab-visibility-chip{font-size:12px;line-height:1;padding:5px 10px;border-radius:var(--radius-pill,999px);border:1px solid var(--border);background:var(--accent-bg);color:var(--accent-text);cursor:pointer;transition:background .15s,border-color .15s,color .15s;white-space:nowrap;user-select:none;}
|
||||
.tab-visibility-chip:hover{border-color:var(--accent);background:var(--accent-bg-strong);}
|
||||
.tab-visibility-chip:focus-visible{outline:2px solid var(--link,var(--accent));outline-offset:2px;}
|
||||
.tab-visibility-chip.chip-off{background:var(--input-bg,var(--surface));color:var(--muted);border-color:var(--border);}
|
||||
.tab-visibility-chip.chip-off:hover{border-color:var(--muted);}
|
||||
|
||||
|
||||
@@ -102,4 +102,62 @@ def test_i18n_coverage():
|
||||
assert label_count >= 11, f"Expected ≥11 locales, found {label_count}"
|
||||
assert desc_count >= 11, f"Expected ≥11 locales, found {desc_count}"
|
||||
assert label_count == desc_count, \
|
||||
f"Label ({label_count}) and desc ({desc_count}) counts must match"
|
||||
f"Label ({label_count}) and desc ({desc_count}) counts must match"
|
||||
|
||||
|
||||
def test_backend_rejects_chat_and_settings_in_hidden_tabs(monkeypatch, tmp_path):
|
||||
"""Server-side belt-and-suspenders: a malicious POST that tries to hide
|
||||
`chat` or `settings` (the always-visible nav tabs) must be filtered out
|
||||
server-side, not just client-side. The client already applies the same
|
||||
filter at apply time, but the server should not let a tampered payload
|
||||
persist the forbidden values."""
|
||||
import api.config as config
|
||||
settings_path = tmp_path / "settings.json"
|
||||
monkeypatch.setattr(config, "SETTINGS_FILE", settings_path)
|
||||
|
||||
saved = config.save_settings({"hidden_tabs": ["chat", "kanban", "settings", "logs"]})
|
||||
assert saved["hidden_tabs"] == ["kanban", "logs"], \
|
||||
"chat and settings must be stripped server-side"
|
||||
|
||||
# Even an all-forbidden payload reduces to empty (not rejected — empty is fine)
|
||||
saved = config.save_settings({"hidden_tabs": ["chat", "settings"]})
|
||||
assert saved["hidden_tabs"] == []
|
||||
|
||||
|
||||
def test_profile_switch_reconciles_hidden_tabs():
|
||||
"""When a user switches profiles, the new profile's hidden_tabs value
|
||||
must be applied — the per-profile settings.json is the source of truth,
|
||||
not the previous profile's localStorage value. Stage-394 added a
|
||||
/api/settings refetch in _refreshProfileSwitchBackground; verify it stays
|
||||
wired (the API call + the _applyTabVisibility call)."""
|
||||
bg_start = PANELS_JS.find("function _refreshProfileSwitchBackground")
|
||||
assert bg_start >= 0, "_refreshProfileSwitchBackground not found"
|
||||
bg_end = PANELS_JS.find("\nfunction ", bg_start + 1)
|
||||
if bg_end < 0:
|
||||
bg_end = bg_start + 4000
|
||||
bg_body = PANELS_JS[bg_start:bg_end]
|
||||
assert "/api/settings" in bg_body, \
|
||||
"profile-switch background refresh must re-fetch settings for the new profile"
|
||||
assert "_applyTabVisibility" in bg_body, \
|
||||
"profile-switch background refresh must re-apply tab visibility"
|
||||
assert "hidden_tabs" in bg_body, \
|
||||
"profile-switch background refresh must read hidden_tabs from server response"
|
||||
|
||||
|
||||
def test_chip_a11y_uses_switch_role_with_aria_checked():
|
||||
"""Chips should use role=switch + aria-checked instead of plain
|
||||
aria-pressed. The pressed/not-pressed wording is confusing for a toggle
|
||||
that visually represents an on/off switch; role=switch + aria-checked
|
||||
matches user mental model."""
|
||||
render_block = PANELS_JS[PANELS_JS.find("function _renderTabVisibilityChips"):]
|
||||
body = render_block[:render_block.find("\nfunction ", 1) or 3000]
|
||||
assert "role" in body and "'switch'" in body, \
|
||||
"chip should declare role='switch' for clearer screen-reader narration"
|
||||
assert "aria-checked" in body, "chip should use aria-checked to match role=switch"
|
||||
# Group container also has role=group + aria-labelledby
|
||||
assert 'role="group"' in INDEX_HTML, "chip container needs role=group"
|
||||
assert 'aria-labelledby="tabVisibilityLabel"' in INDEX_HTML, \
|
||||
"chip container needs aria-labelledby pointing at the label"
|
||||
# Focus-visible style exists
|
||||
assert ".tab-visibility-chip:focus-visible" in STYLE_CSS, \
|
||||
"chip needs a :focus-visible style for keyboard nav"
|
||||
Reference in new issue
Block a user