From 8a1032349c28b5c69591cb79fc7dccce97c28122 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Sat, 31 Oct 2026 09:59:58 -0400 Subject: [PATCH 1/2] fix(recordings): reconcile bulk tags after delayed refreshes --- .../preact/recordings/TagsOverlay.jsx | 59 ++-- web/tests/e2e/bulkTagsReconciliation.spec.js | 292 ++++++++++++++++++ 2 files changed, 332 insertions(+), 19 deletions(-) create mode 100644 web/tests/e2e/bulkTagsReconciliation.spec.js diff --git a/web/js/components/preact/recordings/TagsOverlay.jsx b/web/js/components/preact/recordings/TagsOverlay.jsx index 6892258af..8ac3cce26 100644 --- a/web/js/components/preact/recordings/TagsOverlay.jsx +++ b/web/js/components/preact/recordings/TagsOverlay.jsx @@ -186,28 +186,43 @@ export function BulkTagsOverlay({ recordings, selectedRecordings, onClose, onTag .filter(([_, sel]) => sel) .map(([id]) => parseInt(id, 10)); - const selectedRecs = recordings.filter(r => selectedIds.includes(r.id)); + const selectionKey = JSON.stringify(selectedIds); + const currentSelectionRef = useRef(selectionKey); + currentSelectionRef.current = selectionKey; - // Compute tag states from selected recordings + // The catalog is independent of the recording snapshot. Loading more names + // must not overwrite a successful local edit while the parent refetches rows. useEffect(() => { recordingsAPI.getAllRecordingTags().then(setAllTags); }, []); useEffect(() => { - const states = {}; - // Gather all tags from selected recordings - const allUsedTags = new Set(); - selectedRecs.forEach(r => (r.tags || []).forEach(t => allUsedTags.add(t))); - allTags.forEach(t => allUsedTags.add(t)); + setTagStates(prev => { + const states = { ...prev }; + allTags.forEach(tag => { + if (!Object.hasOwn(states, tag)) states[tag] = 'none'; + }); + return states; + }); + }, [allTags]); - allUsedTags.forEach(tag => { - const count = selectedRecs.filter(r => (r.tags || []).includes(tag)).length; - if (count === 0) states[tag] = 'none'; - else if (count === selectedRecs.length) states[tag] = 'all'; - else states[tag] = 'some'; + // A new recording snapshot is authoritative, including same-count refetches. + // Use selected IDs rather than their count so replacing a selection reconciles + // correctly without rerunning on each newly allocated selection object. + useEffect(() => { + const ids = JSON.parse(selectionKey); + const selectedRecs = recordings.filter(recording => ids.includes(recording.id)); + setTagStates(prev => { + const states = {}; + const knownTags = new Set(Object.keys(prev)); + selectedRecs.forEach(recording => (recording.tags || []).forEach(tag => knownTags.add(tag))); + knownTags.forEach(tag => { + const count = selectedRecs.filter(recording => (recording.tags || []).includes(tag)).length; + states[tag] = count === 0 ? 'none' : count === selectedRecs.length ? 'all' : 'some'; + }); + return states; }); - setTagStates(states); - }, [allTags, selectedRecs.length]); + }, [recordings, selectionKey]); // Close on outside click useEffect(() => { @@ -225,18 +240,22 @@ export function BulkTagsOverlay({ recordings, selectedRecordings, onClose, onTag if (current === 'all') { // Remove from all await recordingsAPI.batchUpdateRecordingTags(selectedIds, [], [tag]); - setTagStates(prev => ({ ...prev, [tag]: 'none' })); + if (currentSelectionRef.current === selectionKey) { + setTagStates(prev => ({ ...prev, [tag]: 'none' })); + } showStatusMessage(t('recordings.tagRemovedFromRecordings', { tag, count: selectedIds.length })); } else { // 'none' or 'some' -> add to all await recordingsAPI.batchUpdateRecordingTags(selectedIds, [tag], []); - setTagStates(prev => ({ ...prev, [tag]: 'all' })); + if (currentSelectionRef.current === selectionKey) { + setTagStates(prev => ({ ...prev, [tag]: 'all' })); + } showStatusMessage(t('recordings.tagAddedToRecordings', { tag, count: selectedIds.length })); } if (onTagsChanged) onTagsChanged(); } catch { /* error shown by API */ } setSaving(false); - }, [tagStates, selectedIds, onTagsChanged]); + }, [tagStates, selectedIds, selectionKey, onTagsChanged]); const addNewTag = useCallback(async (tag) => { const trimmed = tag.trim(); @@ -244,7 +263,9 @@ export function BulkTagsOverlay({ recordings, selectedRecordings, onClose, onTag setSaving(true); try { await recordingsAPI.batchUpdateRecordingTags(selectedIds, [trimmed], []); - setTagStates(prev => ({ ...prev, [trimmed]: 'all' })); + if (currentSelectionRef.current === selectionKey) { + setTagStates(prev => ({ ...prev, [trimmed]: 'all' })); + } if (!allTags.includes(trimmed)) setAllTags(prev => [...prev, trimmed].sort()); setInputValue(''); setShowSuggestions(false); @@ -252,7 +273,7 @@ export function BulkTagsOverlay({ recordings, selectedRecordings, onClose, onTag if (onTagsChanged) onTagsChanged(); } catch { /* error shown by API */ } setSaving(false); - }, [selectedIds, allTags, onTagsChanged]); + }, [selectedIds, selectionKey, allTags, onTagsChanged]); const handleKeyDown = (e) => { if (e.key === 'Enter' && inputValue.trim()) { e.preventDefault(); addNewTag(inputValue); } diff --git a/web/tests/e2e/bulkTagsReconciliation.spec.js b/web/tests/e2e/bulkTagsReconciliation.spec.js new file mode 100644 index 000000000..0440b8b46 --- /dev/null +++ b/web/tests/e2e/bulkTagsReconciliation.spec.js @@ -0,0 +1,292 @@ +/** Real component effects with delayed API and parent recording snapshots. */ +// Run with: npm --prefix web run test:e2e -- --testPathPatterns=bulkTagsReconciliation +const browserTest = it; +async function openBrowser({ + delayedCatalog = false +} = {}) { + const { + chromium + } = require('../../../node_modules/playwright'); + const { + build + } = require('esbuild'); + const path = require('node:path'); + const webRoot = path.resolve(__dirname, '../..'); + const compiled = await build({ + stdin: { + resolveDir: webRoot, + loader: 'jsx', + contents: ` + import { h, render } from 'preact'; import { useState } from 'preact/hooks'; + import { BulkTagsOverlay } from './js/components/preact/recordings/TagsOverlay.jsx'; + window.calls = {requests:[],changed:0,messages:[]}; + window.api = { + getAllRecordingTags:()=>${delayedCatalog ? 'new Promise(resolve=>window.finishCatalog=resolve)' : "Promise.resolve(['shared','partial','known'])"}, + batchUpdateRecordingTags:(...args)=>{window.calls.requests.push(args);return new Promise((resolve,reject)=>{window.succeed=resolve;window.fail=()=>reject(new Error('fixture failure'));});} + }; + function App(){ + const [recordings,setRecordings]=useState([{id:1,tags:['shared','partial']},{id:2,tags:['shared']},{id:3,tags:['other']}]); + const [selected,setSelected]=useState({1:true,2:true});window.refresh=setRecordings;window.select=setSelected; + return h(BulkTagsOverlay,{recordings,selectedRecordings:selected,onClose:()=>{},onTagsChanged:()=>window.calls.changed++}); + } + render(h(App),document.getElementById('root')); + ` + }, + bundle: true, + write: false, + format: 'iife', + jsx: 'automatic', + jsxImportSource: 'preact', + plugins: [{ + name: 'fixture', + setup(build) { + build.onResolve({ + filter: /(?:i18n\.js|recordingsAPI\.jsx|ToastContainer\.jsx)$/ + }, args => ({ + path: args.path, + namespace: 'fixture' + })); + build.onLoad({ + filter: /.*/, + namespace: 'fixture' + }, args => ({ + contents: args.path.endsWith('i18n.js') ? 'export const useI18n=()=>({t:key=>key});' : args.path.endsWith('recordingsAPI.jsx') ? 'export const recordingsAPI={getAllRecordingTags:()=>window.api.getAllRecordingTags(),batchUpdateRecordingTags:(...args)=>window.api.batchUpdateRecordingTags(...args)};' : 'export const showStatusMessage=(...args)=>window.calls.messages.push(args);', + loader: 'js' + })); + } + }] + }); + const browser = await chromium.launch({ + headless: true + }); + const page = await browser.newPage(); + await page.setContent('
'); + await page.addScriptTag({ + content: compiled.outputFiles[0].text + }); + await page.getByRole('checkbox', { + name: 'partial', + exact: true + }).waitFor(); + await settle(page); + return { + browser, + page + }; +} +async function settle(page) { + await page.evaluate(() => new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(() => setTimeout(resolve, 60))))); +} +async function state(page, tag) { + return page.getByRole('checkbox', { + name: tag, + exact: true + }).evaluate(el => ({ + checked: el.checked, + mixed: el.indeterminate, + disabled: el.disabled + })); +} +async function enter(page, tag) { + const field = page.getByPlaceholder('recordings.addNewTagPlaceholder', { + exact: true + }); + await field.fill(tag); + await field.press('Enter'); +} +async function complete(page, success = true) { + await page.evaluate(success => success ? window.succeed() : window.fail(), success); + await settle(page); +} +browserTest('successful new tag survives stale rows and late autocomplete catalog, then reconciles same-count refresh', async () => { + const { + browser, + page + } = await openBrowser({ + delayedCatalog: true + }); + try { + await enter(page, ' new '); + expect(await page.evaluate(() => window.calls.requests)).toEqual([[[1, 2], ['new'], []]]); + await complete(page); + expect(await state(page, 'new')).toMatchObject({ + checked: true, + mixed: false, + disabled: false + }); + await page.evaluate(() => window.finishCatalog(['shared', 'partial', 'catalog-only'])); + await settle(page); + expect(await state(page, 'new')).toMatchObject({ + checked: true, + mixed: false + }); + expect(await state(page, 'catalog-only')).toMatchObject({ + checked: false, + mixed: false + }); + await page.evaluate(() => window.refresh([{ + id: 1, + tags: ['shared', 'new'] + }, { + id: 2, + tags: ['new'] + }, { + id: 3, + tags: ['other'] + }])); + await settle(page); + expect(await state(page, 'new')).toMatchObject({ + checked: true, + mixed: false + }); + expect(await state(page, 'shared')).toMatchObject({ + checked: false, + mixed: true + }); + expect(await state(page, 'partial')).toMatchObject({ + checked: false, + mixed: false + }); + await page.evaluate(() => window.refresh([{ + id: 1, + tags: [] + }, { + id: 2, + tags: ['new'] + }, { + id: 3, + tags: ['other'] + }])); + await settle(page); + expect(await state(page, 'new')).toMatchObject({ + checked: false, + mixed: true + }); + expect(await state(page, 'shared')).toMatchObject({ + checked: false, + mixed: false + }); + expect(await page.evaluate(() => window.calls.changed)).toBe(1); + } finally { + await browser.close(); + } +}, 30000); +browserTest('same-size selection replacement recomputes actual tags and late old-selection completion cannot mark new rows', async () => { + const { + browser, + page + } = await openBrowser(); + try { + await enter(page, 'old-selection'); + await page.evaluate(() => window.select({ + 2: true, + 3: true + })); + await settle(page); + expect(await state(page, 'partial')).toMatchObject({ + checked: false, + mixed: false + }); + expect(await state(page, 'shared')).toMatchObject({ + checked: false, + mixed: true + }); + expect(await state(page, 'other')).toMatchObject({ + checked: false, + mixed: true + }); + await complete(page); + expect(await state(page, 'old-selection')).toMatchObject({ + checked: false, + mixed: false + }); + await page.getByRole('checkbox', { + name: 'other', + exact: true + }).click(); + await complete(page); + expect(await state(page, 'other')).toMatchObject({ + checked: true, + mixed: false + }); + expect(await page.evaluate(() => window.calls.requests)).toEqual([[[1, 2], ['old-selection'], []], [[2, 3], ['other'], []]]); + await page.evaluate(() => window.select({ + 3: true + })); + await settle(page); + expect(await state(page, 'other')).toMatchObject({ + checked: true, + mixed: false + }); + expect(await state(page, 'shared')).toMatchObject({ + checked: false, + mixed: false + }); + } finally { + await browser.close(); + } +}, 30000); +browserTest('failed mixed-add, all-remove and new-tag saves retain state, input and callback counts; pending controls cannot resubmit', async () => { + const { + browser, + page + } = await openBrowser(); + try { + const partial = page.getByRole('checkbox', { + name: 'partial', + exact: true + }); + await partial.focus(); + await page.keyboard.press('Space'); + expect(await partial.isDisabled()).toBe(true); + await partial.evaluate(el => el.click()); + expect(await page.evaluate(() => window.calls.requests.length)).toBe(1); + await complete(page, false); + expect(await state(page, 'partial')).toMatchObject({ + checked: false, + mixed: true, + disabled: false + }); + expect(await page.evaluate(() => window.calls.changed)).toBe(0); + const shared = page.getByRole('checkbox', { + name: 'shared', + exact: true + }); + await shared.click(); + await complete(page, false); + expect(await state(page, 'shared')).toMatchObject({ + checked: true, + mixed: false + }); + await enter(page, 'failed-new'); + expect(await page.getByPlaceholder('recordings.addNewTagPlaceholder', { + exact: true + }).isDisabled()).toBe(true); + await complete(page, false); + expect(await page.getByRole('checkbox', { + name: 'failed-new', + exact: true + }).count()).toBe(0); + expect(await page.getByPlaceholder('recordings.addNewTagPlaceholder', { + exact: true + }).inputValue()).toBe('failed-new'); + expect(await page.evaluate(() => window.calls.changed)).toBe(0); + expect(await page.evaluate(() => window.calls.messages)).toEqual([]); + expect(await page.evaluate(() => window.calls.requests)).toEqual([[[1, 2], ['partial'], []], [[1, 2], [], ['shared']], [[1, 2], ['failed-new'], []]]); + await shared.click(); + await complete(page); + expect(await state(page, 'shared')).toMatchObject({ + checked: false, + mixed: false + }); + await partial.click(); + await complete(page); + expect(await state(page, 'partial')).toMatchObject({ + checked: true, + mixed: false + }); + expect(await page.evaluate(() => window.calls.changed)).toBe(2); + } finally { + await browser.close(); + } +}, 30000); From d6045863aaf5cd9baaf03f88a4bbaae59401c49a Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Sat, 31 Oct 2026 10:03:50 -0400 Subject: [PATCH 2/2] ci: run bulk tag reconciliation on the first shard --- .github/workflows/integration-test.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/integration-test.yml b/.github/workflows/integration-test.yml index f4f825289..907c2eb98 100644 --- a/.github/workflows/integration-test.yml +++ b/.github/workflows/integration-test.yml @@ -604,6 +604,10 @@ jobs: npm ci npx playwright install --with-deps chromium + - name: Bulk tag reconciliation regression + if: ${{ matrix.shard == 1 }} + run: npm --prefix web run test:e2e -- --testPathPatterns=bulkTagsReconciliation + - name: Run Playwright UI tests run: | # Clean any existing playwright reports to ensure fresh state