fix(collections): address review findings

- M1: after a failed Save the Collections dialog shows the membership loading and keeps Save disabled until the re-read lands, so a stale plan cannot be sent. - M2: the changes a Save made reach the page only when the dialog closes, so a collection page's reload cannot remove the dialog before its failure is read. - L4: a New collection name equal to a listed collection ignoring case is that collection's checkbox, checked; never an add then a remove. - L5: a name that cannot be a collection name is refused on the field; nothing is sent, the checkbox edits stay, no re-read. - L6: focus starts on the field and moves to the first checkbox once listed; closing after a re-render replaced the trigger focuses the inspector's current Collections… button or its heading (openContentDialog takes an optional focusTarget). - L3, L7: TESTING.md D138/D139 describe the stale-source and deleted collection outcomes and the above; the viewer spec asserts no listing request while it is open, and a browser test covers M2.
2026-10-27 21:37:13 -07:00
parent 28e0eb1ba6
commit 65b4b2e090
7 changed files with 225 additions and 51 deletions
@@ -78,7 +78,8 @@ export function parseCollections(data) {
 }

 /** The sidebar's order while a re-read is pending: an approximation of the server's name key, then id. */
-const nameKey = name => name.normalize('NFC').toLowerCase().normalize('NFC');
+export const collectionNameKey = name => name.normalize('NFC').toLowerCase().normalize('NFC');
+const nameKey = collectionNameKey;
 const ordered = list => [...list].sort((a, b) => {
    const left = nameKey(a.name), right = nameKey(b.name);
    return left < right ? -1 : left > right ? 1 : a.id - b.id;
@@ -309,9 +309,12 @@ export function mountFileInspector(container, {api, sender, parentScope, variant
        const member = collections ? revealTarget(record.identity) : null;
        if (member) {
            const button = createButton('');
+            button.dataset.collections = '';
            button.append(createWorkspaceIcon('folder'), ' Collections…');
+            // A re-render replaces the button: closing focuses the current one, else the inspector's heading.
+            const fallback = () => container.querySelector('button[data-collections]') ?? container.querySelector('h2');
            contentScope.listen(button, 'click', () => openCollectionsDialog({scope, collections, target:member, trigger:button,
-                onChanged:onCollectionsChanged}));
+                onChanged:onCollectionsChanged, focusFallback:fallback}));
            footer.append(button);
        }
        if (!full && footer.childNodes.length) container.append(footer);
@@ -9,13 +9,19 @@
 * - Save sends one request per change, in order: the new name, each newly checked collection, each unchecked one.
 *   Nothing changed sends nothing and closes. All sent: the dialog closes with "Collections updated.". A failure keeps
 *   it open with "The collections could not be updated. Try again." and the membership read again; the changes that
- *   went through still reach `onChanged({added, removed})` (collection ids), as a full success does.
+ *   went through still reach `onChanged({added, removed})` (collection ids), as a full success does, but only once the
+ *   dialog closes: a page that lists the file again (a collection page) would otherwise remove the dialog before its
+ *   failure is read. Save waits for the membership read again.
+ * - A New collection name equal to a listed collection ignoring case is that collection's checkbox, checked. A name
+ *   that cannot be a collection name is refused on the field; nothing is sent and the checkbox edits stay.
+ * - Focus starts on the field and moves to the first checkbox once they are listed; closing returns it to the
+ *   trigger, or to `focusFallback()` when an inspector re-render removed the trigger.
 */
 import { createButton } from '../shared/toolkit/core.js';
 import { closeWorkspaceDialog, field, openContentDialog } from './workspace-dialogs.mjs';
 import { showWorkspaceToast } from './toast.mjs';
 import { COLLECTIONS_UPDATED_TOAST, MAX_COLLECTION_NAME_LENGTH, MEMBERSHIP_FAILED_TEXT, MEMBERSHIP_LOADING_TEXT,
-    NO_COLLECTIONS_TEXT, UPDATE_FAILED_TEXT, normalizeCollectionName } from './collections.mjs';
+    NO_COLLECTIONS_TEXT, UPDATE_FAILED_TEXT, collectionNameKey, normalizeCollectionName } from './collections.mjs';

 export const COLLECTIONS_DIALOG_TITLE = 'Collections';
 export const NEW_COLLECTION_LABEL = 'New collection';
@@ -32,10 +38,12 @@ function el(tag, text, className) {
 * and `closeDialog` are the dialog seams; `notify` the toast.
 */
 export function openCollectionsDialog({ scope, collections, target, trigger = null, notify = showWorkspaceToast, onChanged = null,
-    openDialog = openContentDialog, closeDialog = closeWorkspaceDialog }) {
+    focusFallback = null, openDialog = openContentDialog, closeDialog = closeWorkspaceDialog }) {
    const local = new AbortController();
    const active = () => !scope.signal.aborted && !local.signal.aborted;
-    let member = null, saving = false;
+    let member = null, saving = false, listed = false;
+    // The confirmed changes, reported once when the dialog closes (review M2).
+    const changes = { added: [], removed: [] };
    const status = el('p', '', 'small');
    const retry = createButton('Retry');
    const list = el('div');
@@ -45,6 +53,7 @@ export function openCollectionsDialog({ scope, collections, target, trigger = nu
    input.value = '';
    input.maxLength = MAX_COLLECTION_NAME_LENGTH;
    input.autocomplete = 'off';
+    input.addEventListener('input', () => input.setCustomValidity?.(''));
    const problem = el('p', '', 'small');
    problem.setAttribute('role', 'alert');
    problem.hidden = true;
@@ -84,53 +93,67 @@ export function openCollectionsDialog({ scope, collections, target, trigger = nu
        status.hidden = !text;
        retry.hidden = !failed;
    }
-    async function load() {
+    /** Save waits while the membership is read (again). */
+    function loading() {
        member = null;
        paintSave();
        list.replaceChildren();
        showStatus(MEMBERSHIP_LOADING_TEXT, false);
+    }
+    async function load() {
+        loading();
        try {
            const ids = await collections.membership(target);
            if (!active()) return;
            member = new Set(ids);
            showStatus('', false);
            renderList();
+            // The first listing takes focus from the untouched field to the first checkbox (review L6).
+            if (!listed && !input.value) firstBox().focus();
+            listed = true;
        } catch {
            if (!active()) return;
            showStatus(MEMBERSHIP_FAILED_TEXT, true);
        }
        paintSave();
    }
-    /** The requests Save sends, in order; null when the new name cannot be a collection name. */
+    const boxesOf = () => [...list.children].map(row => row.children?.[0]).filter(box => box?.dataset?.collectionId);
+    const firstBox = () => boxesOf()[0] ?? input;
+    /**
+     * The requests Save sends, in order; null when the new name cannot be a collection name. A name equal to a listed
+     * collection's ignoring case is that collection's checkbox, checked (review L4).
+     */
    function plan() {
        const typed = input.value.trim();
        const name = typed ? normalizeCollectionName(typed) : '';
        if (name === null) return null;
-        const steps = name ? [{ add: { name } }] : [];
-        const boxes = [...list.children].map(row => row.children?.[0]).filter(box => box?.dataset?.collectionId);
-        for (const box of boxes) {
-            const id = Number(box.dataset.collectionId);
-            if (box.checked && !member.has(id)) steps.push({ add: { collectionId: id } });
-        }
-        for (const box of boxes) {
-            const id = Number(box.dataset.collectionId);
-            if (!box.checked && member.has(id)) steps.push({ remove: id });
-        }
+        const named = name ? collections.collections.find(item => collectionNameKey(item.name) === collectionNameKey(name)) : null;
+        const steps = name && !named ? [{ add: { name } }] : [];
+        const boxes = boxesOf().map(box => ({ id: Number(box.dataset.collectionId), checked: box.checked }))
+            .map(box => (box.id === named?.id ? { ...box, checked: true } : box));
+        for (const box of boxes) if (box.checked && !member.has(box.id)) steps.push({ add: { collectionId: box.id } });
+        for (const box of boxes) if (!box.checked && member.has(box.id)) steps.push({ remove: box.id });
        return steps;
    }
    async function submit() {
        problem.textContent = '';
        problem.hidden = true;
        const steps = plan();
-        if (steps?.length === 0) { closeDialog(); return; }
+        if (steps === null) {
+            // Review L5: refused on the field; nothing is sent and the checkbox edits stay.
+            input.setCustomValidity?.(UPDATE_FAILED_TEXT);
+            input.reportValidity?.();
+            input.focus();
+            return;
+        }
+        if (steps.length === 0) { closeDialog(); return; }
        saving = true;
        paintSave();
-        const added = [], removed = [];
-        let failed = steps === null;
-        for (const step of steps ?? []) {
+        let failed = false;
+        for (const step of steps) {
            try {
-                if (step.add) added.push((await collections.add(step.add, [target])).collection.id);
-                else removed.push((await collections.remove(step.remove, [target])).collection.id);
+                if (step.add) changes.added.push((await collections.add(step.add, [target])).collection.id);
+                else changes.removed.push((await collections.remove(step.remove, [target])).collection.id);
            } catch {
                failed = true;
                break;
@@ -140,7 +163,6 @@ export function openCollectionsDialog({ scope, collections, target, trigger = nu
        }
        if (!active()) return;
        saving = false;
-        if (added.length || removed.length) onChanged?.({ added, removed });
        if (!failed) {
            closeDialog();
            notify(COLLECTIONS_UPDATED_TOAST);
@@ -148,12 +170,20 @@ export function openCollectionsDialog({ scope, collections, target, trigger = nu
        }
        problem.textContent = UPDATE_FAILED_TEXT;
        problem.hidden = false;
-        // A collection deleted meanwhile is re-read by the store; the list then shows what the server holds.
+        // Review M1: Save waits from here until the membership is read again. A collection deleted meanwhile is
+        // re-read by the store; the list then shows what the server holds.
+        loading();
        await collections.settled();
        if (active()) await load();
    }
+    function closed() {
+        local.abort();
+        if (changes.added.length || changes.removed.length) onChanged?.({ added: [...changes.added], removed: [...changes.removed] });
+        // The shell focuses a trigger still on the page; one an inspector re-render replaced falls back (review L6).
+        if (trigger && !trigger.isConnected) focusFallback?.()?.focus();
+    }
    showStatus('', false);
-    openDialog({ scope, title: COLLECTIONS_DIALOG_TITLE, trigger, canClose: () => !saving, onClose: () => local.abort(),
+    openDialog({ scope, title: COLLECTIONS_DIALOG_TITLE, trigger, canClose: () => !saving, onClose: closed, focusTarget: input,
        nodes: [status, retry, list, field(NEW_COLLECTION_LABEL, input, 'collection-new'), problem, actions] });
    load();
 }
@@ -376,13 +376,13 @@ export function openInputDialog({ scope, title, label, action, value = '', maxLe
 * another dialog replaces its body, so the page can dispose what it mounted there. While `canClose()` answers false,
 * × and Escape leave the dialog open (the page, a route change or a replacing dialog still close it).
 */
-export function openContentDialog({ scope, title, nodes, onClose = null, trigger = null, canClose = null }) {
+export function openContentDialog({ scope, title, nodes, onClose = null, trigger = null, canClose = null, focusTarget = null }) {
    requireScope(scope);
    if (typeof title !== 'string' || !title) throw new TypeError('The dialog needs a title');
    if (!Array.isArray(nodes) || nodes.some(node => !node || typeof node !== 'object')) throw new TypeError('The dialog needs its nodes');
    if (onClose !== null && typeof onClose !== 'function') throw new TypeError('The dialog onClose must be a function');
    if (canClose !== null && typeof canClose !== 'function') throw new TypeError('The dialog canClose must be a function');
-    show(title, nodes, { scope, trigger, onClose, canClose });
+    show(title, nodes, { scope, trigger, onClose, canClose, focusTarget });
 }

 export const SCAN_REVIEW_SENTENCE = 'Enrich positive / negative associations, reuse canonical text and report unchanged files. ' +