Skip to content

fix(book-form): make contributor/author/publisher autocomplete race-proof #272

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fabiodalez-dev merged 4 commits into from
Jul 20, 2026
Merged

fix(book-form): make contributor/author/publisher autocomplete race-proof #272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0115166
fix(book-form): make contributor/author/publisher autocomplete race-p…
fabiodalez-dev Jul 19, 2026
3c5b2b2
fix(book-form): clear the dropdown on a no-results search
fabiodalez-dev Jul 19, 2026
b57a7f5
feat(book-form): stack the multi-select search input below the chips
fabiodalez-dev Jul 19, 2026
3d61648
fix: restore internalInput decl in authors picker (stackChoicesInput …
fabiodalez-dev Jul 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 69 additions & 33 deletions app/Views/libri/partials/book_form.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,11 @@ function initializeChoicesJS() {

authorsChoice = new Choices(element, {
searchEnabled: true,
// Server-side search: the 'search' handler below fetches matches and
// setChoices()es them. Disable Choices' own client-side Fuse filter so
// it can't race that async populate and hide fresh results as "no
// results" until the next keystroke.
searchChoices: false,
removeItemButton: true,
addItems: true,
duplicateItemsAllowed: false,
Expand Down Expand Up @@ -1417,43 +1422,27 @@ classNames: {
customProperties: { isNew: false }
}));

if (newChoices.length > 0) {
authorsChoice.setChoices(newChoices, 'value', 'label', false);
}
// Replace unconditionally (even when empty): with the client
// Fuse filter off, skipping the call on a no-results query
// would leave the PREVIOUS query's options stale in the
// dropdown. clearSearchFlag=false keeps the user's input.
authorsChoice.setChoices(newChoices, 'value', 'label', true, false);
} catch (e) {
console.error('Server-side author search failed:', e);
}
}, 300);
});

const wrapper = element.closest('.choices');
// Load-bearing for the #74 _onEnterKey monkey-patch below (and the
// ensureAuthorChoice helpers): the authors block references
// `internalInput` in several places. It used to be declared by the
// now-removed forceInputWidth block — keep it here so replacing that
// block with stackChoicesInput() doesn't leave it undefined.
const internalInput = wrapper ? wrapper.querySelector('.choices__input--cloned') : null;

// Force input to take remaining space, overriding Choices.js inline styles
if (internalInput) {
const forceInputWidth = () => {
internalInput.style.flex = '1 1 auto';
internalInput.style.minWidth = '200px';
internalInput.style.width = 'auto';
};

// Initial force
forceInputWidth();

// Watch for Choices.js changing the width
const observer = new MutationObserver((mutations) => {
mutations.forEach((mutation) => {
if (mutation.type === 'attributes' && mutation.attributeName === 'style') {
forceInputWidth();
}
});
});

observer.observe(internalInput, {
attributes: true,
attributeFilter: ['style']
});
}
// Stack the search input on its own full-width row below the chips
// (shared with the contributor + publisher pickers).
stackChoicesInput(wrapper);

const ensureAuthorChoice = (value, label, customProperties = {}) => {
const stringValue = String(value);
Expand Down Expand Up @@ -2293,6 +2282,33 @@ classNames: { containerInner: 'choices__inner' }
});
}

// Multi-select tag inputs: wrap the selected chips across rows and drop the
// search field onto its own full-width line BELOW them, so you type UNDER the
// chips instead of being squeezed to the right of the last one. Applied as
// inline !important (the compiled Choices CSS otherwise wins) and re-applied
// via a MutationObserver because Choices.js autosizes the input's inline width
// on every keystroke, which would otherwise pull it back onto the chip row.
function stackChoicesInput(wrapper) {
if (!wrapper) return;
const inner = wrapper.querySelector('.choices__inner');
if (inner) {
inner.style.setProperty('flex-wrap', 'wrap', 'important');
inner.style.setProperty('align-items', 'flex-start', 'important');
}
const input = wrapper.querySelector('.choices__input--cloned');
if (!input) return;
const apply = () => {
input.style.setProperty('flex', '1 0 100%', 'important');
input.style.setProperty('min-width', '100%', 'important');
input.style.setProperty('width', '100%', 'important');
input.style.setProperty('margin', '6px 8px 2px', 'important');
};
apply();
// Re-apply identical values when Choices rewrites the style attribute; setting
// the same values produces no further mutation record, so this settles at once.
new MutationObserver(() => apply()).observe(input, { attributes: true, attributeFilter: ['style'] });
}

// Generic contributor picker (issue #237) — one Choices.js entity picker per
// role (illustratori/traduttori/curatori/coloristi). Mirrors the authors picker
// (same /api/search/autori autocomplete, create-on-Enter) but self-contained and
Expand All @@ -2306,6 +2322,10 @@ function initContributorPicker(roleKey) {

const choice = new Choices(el, {
searchEnabled: true,
// Server-side search (see the 'search' handler below): disable the
// client-side Fuse filter so it can't race the async setChoices and
// hide fresh results as "no results".
searchChoices: false,
removeItemButton: true,
addItems: true,
duplicateItemsAllowed: false,
Expand Down Expand Up @@ -2431,12 +2451,19 @@ function syncHidden(value, label, add) {
const newChoices = (results || [])
.filter((a) => !selected.has(String(a.id)))
.map((a) => ({ value: String(a.id), label: a.label, selected: false }));
if (newChoices.length > 0) choice.setChoices(newChoices, 'value', 'label', false);
// Replace unconditionally (even when empty): with the client
// Fuse filter off, skipping the call on a no-results query
// would leave the PREVIOUS query's options stale in the
// dropdown. clearSearchFlag=false keeps the user's input.
choice.setChoices(newChoices, 'value', 'label', true, false);
} catch (e) {
console.error('Contributor search failed (' + roleKey + '):', e);
}
}, 300);
});

// Search input on its own full-width row below the selected chips.
stackChoicesInput(wrapper);
return true;
} catch (error) {
console.error('initContributorPicker failed for ' + roleKey + ':', error);
Expand All @@ -2453,6 +2480,10 @@ function initializePublishersChoices() {

publishersChoice = new Choices(element, {
searchEnabled: true,
// Server-side search (see the 'search' handler below): disable the
// client-side Fuse filter so it can't race the async setChoices and
// hide fresh results as "no results".
searchChoices: false,
removeItemButton: true,
addItems: true,
duplicateItemsAllowed: false,
Expand Down Expand Up @@ -2483,9 +2514,11 @@ classNames: { containerInner: 'choices__inner' }
const newChoices = (serverResults || [])
.filter(p => !selectedValues.has(String(p.id)))
.map(p => ({ value: String(p.id), label: p.label, selected: false, customProperties: { isNew: false } }));
if (newChoices.length > 0) {
publishersChoice.setChoices(newChoices, 'value', 'label', false);
}
// Replace unconditionally (even when empty): with the client
// Fuse filter off, skipping the call on a no-results query
// would leave the PREVIOUS query's options stale in the
// dropdown. clearSearchFlag=false keeps the user's input.
publishersChoice.setChoices(newChoices, 'value', 'label', true, false);
} catch (e) {
console.error('Server-side publisher search failed:', e);
}
Expand All @@ -2495,6 +2528,9 @@ classNames: { containerInner: 'choices__inner' }
const pubWrapper = element.closest('.choices');
const pubInternalInput = pubWrapper ? pubWrapper.querySelector('.choices__input--cloned') : null;

// Search input on its own full-width row below the selected chips.
stackChoicesInput(pubWrapper);

/**
* Create a NEW publisher from typed text. A <select multiple> Choices.js
* does not natively add free-text options, so (like the authors field)
Expand Down
110 changes: 110 additions & 0 deletions tests/contributor-autocomplete-config.unit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
<?php
declare(strict_types=1);

/**
* Regression guard for the Choices.js server-side-search pickers on the book form
* (authors, contributors, publishers).
*
* These pickers fetch matches from /api/search/autori on keystroke and feed them
* in via setChoices(). Choices.js's OWN client-side Fuse filter (searchChoices,
* default true) races that async populate: for a short/partial query it renders
* the dropdown ("no results") BEFORE the fetch lands and never re-filters, so
* fresh server results are hidden until the next keystroke. The fix disables the
* client filter (searchChoices: false) and makes each server response REPLACE the
* previous query's options WITHOUT resetting the user's in-progress input
* (setChoices(..., replaceChoices=true, clearSearchFlag=false)) — so results
* always show, no stale options linger, and neither the typed text nor an
* already-selected chip is cleared.
*
* The race itself is timing-dependent and can't be asserted deterministically, so
* this locks in the structural fix: a revert (dropping searchChoices:false, or
* flipping the setChoices flags back) fails here. Behaviour is covered by manual
* E2E on the live form.
*
* Run: php tests/contributor-autocomplete-config.unit.php (exit 0 iff all pass)
*/

$root = dirname(__DIR__);
$src = (string) file_get_contents($root . '/app/Views/libri/partials/book_form.php');

$pass = 0;
$fail = 0;
$check = static function (bool $ok, string $label) use (&$pass, &$fail): void {
if ($ok) { $pass++; echo " OK {$label}\n"; }
else { $fail++; echo " FAIL {$label}\n"; }
};

// Normalise whitespace so multi-line / re-indented matches still hold.
$flat = (string) preg_replace('/\s+/', ' ', $src);

echo "A. Client-side Fuse filter disabled on the three server-search pickers\n";
// Each picker's `new Choices(...)` config must carry `searchChoices: false`.
$configs = [
'authors' => 'authorsChoice = new Choices(element, {',
'contributors' => 'const choice = new Choices(el, {',
'publishers' => 'publishersChoice = new Choices(element, {',
];
foreach ($configs as $name => $ctor) {
$pos = strpos($flat, $ctor);
// Look at the ~400 chars following the constructor for searchChoices:false.
$window = $pos !== false ? substr($flat, $pos, 400) : '';
$check($pos !== false && (bool) preg_match('/searchChoices:\s*false/', $window),
"{$name} picker sets searchChoices: false");
}

echo "B. Server results REPLACE options without resetting the search input\n";
// The three search-handler setChoices(newChoices, ...) calls must pass
// replaceChoices=true AND clearSearchFlag=false — i.e. `, true, false)`.
$searchSetChoices = [
'authors' => 'authorsChoice.setChoices(newChoices',
'contributors' => 'choice.setChoices(newChoices',
'publishers' => 'publishersChoice.setChoices(newChoices',
];
foreach ($searchSetChoices as $name => $call) {
// Match the specific call and assert its trailing args are `, true, false)`.
$ok = (bool) preg_match(
'/' . preg_quote($call, '/') . ",\s*'value',\s*'label',\s*true,\s*false\s*\)/",
$flat
);
$check($ok, "{$name} search handler uses setChoices(..., replace=true, clearSearchFlag=false)");
}

echo "C. No server-search handler still uses the racy append form\n";
// Guard against a partial revert: none of the three newChoices setChoices calls
// may use the old 4-arg append form `, 'value', 'label', false)`.
foreach ($searchSetChoices as $name => $call) {
$regressed = (bool) preg_match(
'/' . preg_quote($call, '/') . ",\s*'value',\s*'label',\s*false\s*\)/",
$flat
);
$check(!$regressed, "{$name} search handler does NOT use the old append form (racy)");
}

echo "D. Server results are applied unconditionally (clears stale on no-results)\n";
// With the client filter off, a `if (newChoices.length > 0)` guard before
// setChoices would leave the previous query's options visible when a search
// returns nothing. The setChoices must run even for an empty array so replace
// clears the dropdown (CodeRabbit #272). No such guard may remain.
$check(!str_contains($src, 'newChoices.length > 0'),
'no `if (newChoices.length > 0)` guard gates the server-search setChoices calls');

echo "E. Authors block declares `internalInput` used by its #74 _onEnterKey patch\n";
// Regression guard: the stackChoicesInput refactor replaced the authors-only
// forceInputWidth block, which used to declare `const internalInput`. The
// authors picker's _onEnterKey monkey-patch (load-bearing for issue #74) and
// its ensureAuthorChoice helpers reference `internalInput`; without the
// declaration in scope the whole picker throws "ReferenceError: internalInput
// is not defined" on the first keystroke, so adding an author via Enter — and
// the entire book form's JS — breaks. Assert the declaration sits between the
// authors `new Choices(...)` and its `_onEnterKey` patch.
$authPos = strpos($flat, 'authorsChoice = new Choices(element, {');
$enterPos = strpos($flat, 'authorsChoice._onEnterKey = function');
$authInternalOk = $authPos !== false && $enterPos !== false && $authPos < $enterPos
&& (bool) preg_match(
'/authorsChoice = new Choices\(element, \{.*?const internalInput\s*=\s*wrapper\s*\?\s*wrapper\.querySelector/s',
$src
);
$check($authInternalOk, 'authors picker declares `const internalInput` before its _onEnterKey patch');

echo "\n" . ($fail === 0 ? "ALL {$pass} PASS\n" : "{$pass} PASS, {$fail} FAIL\n");
exit($fail === 0 ? 0 : 1);
Loading