mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-09 16:59:43 +02:00
fix(tabs): collapsed-header alerts, quieter layout reads, tree key and touch fixes (#517, #519 review)
Maintainer merge-time fixes for the grouped vertical rail (#517) and its tree semantics (#519), from the two PR reviews. #517 minors - A collapsed group hid rows that need the user with no signal on its header. The header now takes the most urgent alert among the session rows its collapse hides, in the tab alert language (tab-alert-action red ring, tab-alert-idle yellow ring, the existing ::before rules extended to the header). New pure hiddenGroupAlerts() over a per-section `hidden` list; _syncTabGroupHeaderAlerts() patches it on BOTH render paths, since alerts change without a rebuild. The kept selection draws its own ring and is not counted. - Every layout read rebuilt the whole tab strip, and failed reads retried every 5 s forever. _applyTabLayout() now rebuilds only when the structure key changed. The key drops the layout version (bumped on every session create/close and order PUT) and instead carries group names and the rows each collapse hides, so a version bump that moves nothing costs nothing and a rename still rebuilds. The load coordinator backs off (5, 10, 20, 40 s, capped at 60 s) and stops after 4 retries; the next SSE init or tab:layoutChanged tries again, a success resets. - A malformed stored collapse value disabled collapse on that device for good. A parse or shape error now reads as nothing collapsed and is rewritten to []; ok:false stays reserved for a store that throws. - Ctrl+Shift+{ / } still reordered across groups, where the server re-ranks per group, sends no session:orderChanged and leaves this client's sessionOrder and Alt+N targets diverged. The move is now a no-op unless the neighbour is in the active session's own section (_canSwapActiveTabWith, reading the projection's new sectionByRef, which also covers rows a collapse hides). Within a group the swap still works and the server agrees with it; the flat rail and the strip are unchanged. #517 nits - Keyboard group toggle dropping focus: already fixed by #519's focus-by-identity; the Enter toggle test now pins focus on the header. - Header <button> inside role=tablist: moot, #519 made the header a treeitem inside role=tree. - Byte-identity test not comparing against master: skipped in the suite (a test cannot read another revision's files portably). Checked by hand instead: the flat strip and flat rail markup of this branch before and after this commit are identical in all 16 cases (both orientations, manual and activity sort, no layout and zero groups, full and incremental paths). - Doubled blank line in docs/architecture-invariants.md: removed. #519 minors - A tap on a tree header or unselected row dismissed the touch keyboard: the roving tabindex parks those at -1, so the [tabindex] arm of MOBILE_KEYBOARD_DISMISS_EXEMPT_SELECTOR missed them. The selector now lists [role="treeitem"]. - The tree key handler acted on keys pressed on a focused control inside a row (Enter on the overflow button re-selected and reloaded the active session instead of reopening its menu). It now returns unless the key landed on the treeitem itself. #519 nits - aria-posinset/setsize went stale when the activity-sorted grouped rail re-sorted rows on the incremental path. The position pass is extracted (_applyTabTreePositions) and re-run, with aria-selected and the header alerts, at the end of the incremental branch while the rail is a tree. - An expanded group with no open rows was announced as an expanded parent owning an empty group. A group with no open rows is now a tree leaf: no aria-expanded, no aria-owns, its rows container presentation; Left and Right do nothing on it, and its chevron keys off the section's collapsed class instead of aria-expanded. Tests: tab-layout-browser (malformed storage, backoff with a bounded drain, structure key, hidden alerts, leaf groups, sectionByRef), tab-layout-rail (header alerts on both paths, render-on-change, backoff without rebuilds, malformed storage, Ctrl+Shift section gate, in-row control keys, leaf header keys, posinset after an incremental re-sort, the dismiss selector matching tree items), and three new Chromium tests in tab-activation.browser (Enter on a focused overflow button, the touch keyboard staying up on tree taps, the collapsed header's red ring). Every new test fails on the pre-fix sources. Docs: architecture-invariants owner-tab-layouts and keyboard-dismissal sections, one clause in CLAUDE.md's dismissal rule. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
@@ -11,7 +11,7 @@
|
||||
* are rendered by the caller (app.js, webview-tabs.js), so a grouped row is
|
||||
* byte-identical to the flat rail's row.
|
||||
* 3. Load sequencing: concurrent layout reads settle newest-wins, and a failed
|
||||
* read degrades to the flat rail with a bounded retry.
|
||||
* read degrades to the flat rail with a capped, backed-off retry.
|
||||
*
|
||||
* The server stays the only authority for layout content. Collapse is a
|
||||
* per-device view preference and lives in localStorage only.
|
||||
@@ -61,18 +61,34 @@
|
||||
return !!layout && Array.isArray(layout.groups) && layout.groups.length > 0;
|
||||
}
|
||||
|
||||
/** Stored collapse ids, or null when the stored value is not a JSON array. */
|
||||
function parseCollapsedIds(raw) {
|
||||
if (raw === null) return [];
|
||||
try {
|
||||
const parsed = JSON.parse(raw);
|
||||
return Array.isArray(parsed) ? stableIds(parsed) : null;
|
||||
} catch (_error) {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Read the per-device collapse ids. `ok: false` means the STORE failed (a read
|
||||
* or write threw), and the caller then keeps every group expanded. A malformed
|
||||
* VALUE is not a store failure: it reads as "nothing collapsed" and is
|
||||
* rewritten, or a shape left behind by another build (a rollback) would leave
|
||||
* collapse disabled on this device for good.
|
||||
*/
|
||||
function loadCollapsedGroupIds(storage, validGroupIds) {
|
||||
try {
|
||||
const raw = storage.getItem(COLLAPSED_STORAGE_KEY);
|
||||
const parsed = raw === null ? [] : JSON.parse(raw);
|
||||
if (!Array.isArray(parsed)) throw new Error('Invalid collapsed tab groups');
|
||||
const loaded = stableIds(parsed);
|
||||
const parsed = parseCollapsedIds(storage.getItem(COLLAPSED_STORAGE_KEY));
|
||||
const loaded = parsed || [];
|
||||
if (validGroupIds === undefined) return { ids: loaded, ok: true };
|
||||
// Garbage-collect ids of groups that no longer exist, so a deleted group's
|
||||
// id cannot silently collapse a future group that reuses it.
|
||||
const valid = new Set(stableIds(validGroupIds));
|
||||
const kept = loaded.filter((id) => valid.has(id));
|
||||
if (kept.length !== loaded.length) storage.setItem(COLLAPSED_STORAGE_KEY, JSON.stringify(kept));
|
||||
if (!parsed || kept.length !== loaded.length) storage.setItem(COLLAPSED_STORAGE_KEY, JSON.stringify(kept));
|
||||
return { ids: kept, ok: true };
|
||||
} catch (_error) {
|
||||
return { ids: [], ok: false };
|
||||
@@ -102,8 +118,11 @@
|
||||
* tab, else the active session), so selecting a hidden session by keyboard,
|
||||
* palette or Alt+N never leaves the user with no visible selection.
|
||||
*
|
||||
* @returns {null | { sections, visibleRefs, hiddenTabGroupByRef }} null when the
|
||||
* layout has no groups: the caller renders the flat rail unchanged.
|
||||
* @returns {null | { sections, visibleRefs, hiddenTabGroupByRef, sectionByRef }}
|
||||
* null when the layout has no groups: the caller renders the flat rail
|
||||
* unchanged. Each section lists the rows it shows (`refs`) and the rows its
|
||||
* collapse hides (`hidden`); `sectionByRef` maps every placed row
|
||||
* (`<kind>:<id>`) to its section id (null = Ungrouped), shown or hidden.
|
||||
*/
|
||||
function project(layoutInput, options = {}) {
|
||||
if (!layoutInput) return null;
|
||||
@@ -123,31 +142,35 @@
|
||||
const placed = new Set();
|
||||
const visibleRefs = [];
|
||||
const hiddenTabGroupByRef = {};
|
||||
const sectionByRef = {};
|
||||
const sections = [];
|
||||
|
||||
const place = (refs, sectionId, isCollapsed) => {
|
||||
const shown = [];
|
||||
const hidden = [];
|
||||
let count = 0;
|
||||
for (const ref of refs) {
|
||||
const key = refKey(ref);
|
||||
if (placed.has(key) || !renderable(ref)) continue;
|
||||
placed.add(key);
|
||||
sectionByRef[key] = sectionId;
|
||||
count++;
|
||||
const copy = { kind: ref.kind, id: ref.id };
|
||||
if (isCollapsed && key !== highlighted) {
|
||||
hiddenTabGroupByRef[key] = sectionId;
|
||||
hidden.push(copy);
|
||||
continue;
|
||||
}
|
||||
const copy = { kind: ref.kind, id: ref.id };
|
||||
shown.push(copy);
|
||||
visibleRefs.push(copy);
|
||||
}
|
||||
return { shown, count };
|
||||
return { shown, hidden, count };
|
||||
};
|
||||
|
||||
for (const group of layout.groups) {
|
||||
const isCollapsed = collapsed.has(group.id);
|
||||
const { shown, count } = place(group.refs, group.id, isCollapsed);
|
||||
sections.push({ id: group.id, name: group.name, refs: shown, count, collapsed: isCollapsed });
|
||||
const { shown, hidden, count } = place(group.refs, group.id, isCollapsed);
|
||||
sections.push({ id: group.id, name: group.name, refs: shown, hidden, count, collapsed: isCollapsed });
|
||||
}
|
||||
const omissions = [
|
||||
...liveSessionIds.map((id) => ({ kind: 'session', id })),
|
||||
@@ -155,22 +178,64 @@
|
||||
];
|
||||
const ungrouped = place([...layout.ungrouped, ...omissions], null, false);
|
||||
if (ungrouped.count > 0) {
|
||||
sections.push({ id: null, name: '', refs: ungrouped.shown, count: ungrouped.count, collapsed: false });
|
||||
sections.push({
|
||||
id: null,
|
||||
name: '',
|
||||
refs: ungrouped.shown,
|
||||
hidden: [],
|
||||
count: ungrouped.count,
|
||||
collapsed: false,
|
||||
});
|
||||
}
|
||||
return { sections, visibleRefs, hiddenTabGroupByRef };
|
||||
return { sections, visibleRefs, hiddenTabGroupByRef, sectionByRef };
|
||||
}
|
||||
|
||||
const ALERT_RANK = { action: 2, idle: 1 };
|
||||
|
||||
/**
|
||||
* The most urgent alert behind each COLLAPSED header: `{ [groupId]: 'action' |
|
||||
* 'idle' }` over the session rows the collapse hides. A shown row (the kept
|
||||
* selection, any expanded group) draws its own alert, so it is not counted
|
||||
* here. `alertOf(sessionId)` is the caller's tab alert lookup.
|
||||
*/
|
||||
function hiddenGroupAlerts(projection, alertOf) {
|
||||
const result = {};
|
||||
const sections = projection && Array.isArray(projection.sections) ? projection.sections : [];
|
||||
for (const section of sections) {
|
||||
if (section.id === null || !Array.isArray(section.hidden)) continue;
|
||||
let best = null;
|
||||
for (const ref of section.hidden) {
|
||||
if (ref.kind !== 'session') continue;
|
||||
const alert = alertOf(ref.id);
|
||||
if (ALERT_RANK[alert] && (!best || ALERT_RANK[alert] > ALERT_RANK[best])) best = alert;
|
||||
}
|
||||
if (best) result[section.id] = best;
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
* Everything that changes the grouped rail's STRUCTURE (which rows exist and
|
||||
* where), as opposed to a row's own status/name/badges. The incremental render
|
||||
* path only patches rows in place, so a change here forces a full rebuild.
|
||||
* where, the headers' names, what a collapse hides), as opposed to a row's own
|
||||
* status/name/badges. The incremental render path only patches rows in place,
|
||||
* so a change here forces a full rebuild.
|
||||
*
|
||||
* Deliberately NOT the layout version: the server bumps it on every session
|
||||
* create/close and order PUT, and a bump that moves nothing visible must not
|
||||
* cost every client a full tab-strip rebuild. `layout` is accepted for
|
||||
* signature stability only.
|
||||
*/
|
||||
function structureKey(layout, projection, collapsedGroupIds) {
|
||||
function structureKey(_layout, projection, collapsedGroupIds) {
|
||||
if (!projection) return null;
|
||||
return JSON.stringify({
|
||||
version: layout && Number.isSafeInteger(layout.version) ? layout.version : null,
|
||||
collapsed: stableIds(collapsedGroupIds).sort(),
|
||||
sections: projection.sections.map((section) => [section.id, section.count, section.refs.map(refKey)]),
|
||||
sections: projection.sections.map((section) => [
|
||||
section.id,
|
||||
section.name,
|
||||
section.count,
|
||||
section.refs.map(refKey),
|
||||
(section.hidden || []).map(refKey),
|
||||
]),
|
||||
});
|
||||
}
|
||||
|
||||
@@ -186,6 +251,8 @@
|
||||
* (the rows sit below the header visually, not inside it).
|
||||
* - a COLLAPSED group owns nothing: the one row it still shows (the
|
||||
* selection) is a level-1 sibling, never the child of a closed node.
|
||||
* - a group with NO open rows is a leaf: no `aria-expanded`, no owned group,
|
||||
* so it is not announced as an expanded parent of an empty group.
|
||||
* - Ungrouped rows are level-1 items. Their "Ungrouped" heading is a visual
|
||||
* divider only, hidden from assistive tech, and its rows are not a group.
|
||||
*/
|
||||
@@ -204,10 +271,12 @@
|
||||
const id = escapeHtml(section.id);
|
||||
const refsId = `tab-layout-group-refs-${index}`;
|
||||
const nameId = `tab-layout-group-name-${index}`;
|
||||
const expanded = !section.collapsed;
|
||||
const leaf = section.count === 0;
|
||||
const expanded = !section.collapsed && !leaf;
|
||||
const expandedAttr = leaf ? '' : ` aria-expanded="${expanded ? 'true' : 'false'}"`;
|
||||
return (
|
||||
`<section class="tab-layout-group${section.collapsed ? ' tab-layout-group--collapsed' : ''}" role="presentation" data-tab-group-id="${id}">` +
|
||||
`<div class="tab-layout-group-header tab-layout-group-toggle" role="treeitem" tabindex="-1" data-tab-group-header="${id}" aria-expanded="${expanded ? 'true' : 'false'}"${expanded ? ` aria-owns="${refsId}"` : ''} onclick="app.toggleTabGroupCollapsed(this.dataset.tabGroupHeader)">` +
|
||||
`<div class="tab-layout-group-header tab-layout-group-toggle" role="treeitem" tabindex="-1" data-tab-group-header="${id}"${expandedAttr}${expanded ? ` aria-owns="${refsId}"` : ''} onclick="app.toggleTabGroupCollapsed(this.dataset.tabGroupHeader)">` +
|
||||
'<span class="tab-layout-group-chevron" aria-hidden="true"></span>' +
|
||||
`<span class="tab-layout-group-name" id="${nameId}" data-i18n-skip>${escapeHtml(section.name)}</span>` +
|
||||
`<span class="tab-layout-group-count">${section.count}</span></div>` +
|
||||
@@ -221,11 +290,21 @@
|
||||
* Newest-wins layout loading. A response that was overtaken by a later load is
|
||||
* dropped; a failure applies the fallback (the flat rail) and schedules ONE
|
||||
* retry, replacing any retry already pending.
|
||||
*
|
||||
* Retries back off and stop: the delay doubles from `retryDelayMs` up to
|
||||
* `maxRetryDelayMs`, and after `maxRetries` consecutive failures nothing more
|
||||
* is scheduled (`scheduleRetry(fn, delayMs)`). The next outside load (an SSE
|
||||
* reconnect re-runs init, a `tab:layoutChanged` re-reads) tries again, and
|
||||
* any success resets the count.
|
||||
*/
|
||||
function createLoadCoordinator(options) {
|
||||
const baseDelay = Number.isFinite(options.retryDelayMs) ? options.retryDelayMs : 5000;
|
||||
const maxDelay = Number.isFinite(options.maxRetryDelayMs) ? options.maxRetryDelayMs : 60000;
|
||||
const maxRetries = Number.isSafeInteger(options.maxRetries) ? options.maxRetries : 4;
|
||||
let generation = 0;
|
||||
let disposed = false;
|
||||
let retryHandle = null;
|
||||
let failures = 0;
|
||||
const clearRetry = () => {
|
||||
if (retryHandle !== null && options.cancelRetry) options.cancelRetry(retryHandle);
|
||||
retryHandle = null;
|
||||
@@ -237,12 +316,17 @@
|
||||
try {
|
||||
const layout = await options.fetchLayout();
|
||||
if (disposed || requestGeneration !== generation) return false;
|
||||
failures = 0;
|
||||
options.applyLayout(layout);
|
||||
return true;
|
||||
} catch (_error) {
|
||||
if (disposed || requestGeneration !== generation) return false;
|
||||
failures++;
|
||||
options.applyFallback();
|
||||
retryHandle = options.scheduleRetry(() => load());
|
||||
if (failures <= maxRetries) {
|
||||
const delay = Math.min(maxDelay, baseDelay * 2 ** (failures - 1));
|
||||
retryHandle = options.scheduleRetry(() => load(), delay);
|
||||
}
|
||||
return false;
|
||||
}
|
||||
};
|
||||
@@ -260,6 +344,7 @@
|
||||
normalizeLayout,
|
||||
hasGroups,
|
||||
project,
|
||||
hiddenGroupAlerts,
|
||||
structureKey,
|
||||
renderProjection,
|
||||
createLoadCoordinator,
|
||||
|
||||
Reference in New Issue
Block a user