Conversation
… safely rebuild Collection in store Agent-Logs-Url: https://github.com/SolidOS/solid-ui/sessions/b5b014b0-8b31-48b8-8b6e-38a780fe55b5 Co-authored-by: SharonStrats <9412507+SharonStrats@users.noreply.github.com>
…ers per code review Agent-Logs-Url: https://github.com/SolidOS/solid-ui/sessions/b5b014b0-8b31-48b8-8b6e-38a780fe55b5 Co-authored-by: SharonStrats <9412507+SharonStrats@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a workaround in src/widgets/forms.js for issue #737, where ordered ui:Multiple fields were producing duplicate RDF list heads after a document re-fetch. createListIfNecessary now recovers an existing Collection from the store before allocating a new one, saveListThenRefresh rebuilds the Collection from the current list.elements and removes any stale list-head statements via statementsMatching + removeStatement to avoid rdflib's element-content indexing pitfall, and refresh re-binds list to the store's current list head. New Jest tests cover these paths, and __mocks__/rdflib.ts gains a Fetcher.putBack stub so the tests can exercise the save flow.
Changes:
- Patch
createListIfNecessary,saveListThenRefresh, andrefreshinforms.jsto prevent duplicate ordered-list heads after re-fetch. - Add
test/unit/widgets/forms/multiple.test.tscovering recovery, refresh, add, and dedup-on-save paths. - Mock
Fetcher.putBackin__mocks__/rdflib.ts; addlit-htmltodependenciesinpackage.json(apparently unrelated to the stated PR purpose).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/widgets/forms.js | Recover existing list head, rebuild Collection before save, sync list in refresh. |
| test/unit/widgets/forms/multiple.test.ts | New unit tests for ordered Multiple-field list-head behavior. |
| mocks/rdflib.ts | Adds putBack stub on Fetcher so the new tests can run. |
| package.json | Adds lit-html as a direct dependency (unrelated to the issue). |
Comments suppressed due to low confidence (2)
src/widgets/forms.js:537
saveListThenRefreshnow unconditionally removes the existing list-head triple(s) and adds a freshly-constructedCollectionon every call, even when no element changed and there were no duplicate heads. Ifkb.fetcher.putBack(dataDoc)subsequently fails, the in-memory store has already been mutated (old statement removed, new statement added), leaving the local store out of sync with the remote document. Consider either (a) only rebuilding when duplicates are detected orlist.elementshas actually changed, or (b) restoring the previous statement on putBack failure.
This issue also appears on line 544 of the same file.
const currentElements = list.elements.slice()
const oldStatements = reverse
? kb.statementsMatching(null, property, subject, dataDoc)
: kb.statementsMatching(subject, property, null, dataDoc)
oldStatements.forEach(st => kb.removeStatement(st))
list = new $rdf.Collection(currentElements)
if (reverse) {
kb.add(list, property, subject, dataDoc)
} else {
kb.add(subject, property, list, dataDoc)
}
try {
await kb.fetcher.putBack(dataDoc)
} catch (err) {
box.appendChild(
errorMessageBlock(dom, 'Error trying to put back a list: ' + err)
)
return
}
src/widgets/forms.js:548
- In
refresh(),kb.the(subject, property, null, dataDoc)is used to fetch the single list head. When the duplicate-head bug scenario is in effect (twoCollectionstatements for the same subject/property),kb.thein rdflib emits a warning and arbitrarily picks one — meaning the displayed list may flip between the two collections depending on internal ordering, and the user may see stale or unexpected content prior to the next save. Consider usingkb.each(...)and explicitly picking/merging, or detecting and consolidating duplicates here as well rather than only insaveListThenRefresh.
const li = reverse ? kb.the(null, property, subject, dataDoc) : kb.the(subject, property, null, dataDoc)
if (li) {
list = li // Keep list in sync with the store after any external document reload
}
vals = li ? li.elements : []
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@noble/hashes": "^2.0.1", | ||
| "escape-html": "^1.0.3", | ||
| "lit": "^3.3.2", | ||
| "lit-html": "^3.3.3", |
| @@ -490,6 +497,36 @@ field[ns.ui('Multiple').uri] = function ( | |||
| debug.log('save list: ' + debugString(list.elements)) // 20191214 | |||
|
|
|||
| createListIfNecessary() | |||
|
|
|||
| // Re-create the Collection from the current elements before saving. | |||
| // | |||
| // rdflib's IndexedFormula indexes Collections by their *element content* | |||
| // (not by object identity). After list.elements is mutated (by addItem, | |||
| // deleteThisItem, or moveThisItem), the existing store statement is indexed | |||
| // under the OLD element content key. Calling removeMany() — which uses the | |||
| // CURRENT (mutated) element content to look up the index entry — fails to | |||
| // find it and throws "Statement to be removed is not on store". | |||
| // | |||
| // Calling statementsMatching() with a null object finds the statement via | |||
| // the subject/predicate index (bypassing the stale object index), and | |||
| // removeStatement() removes it from the statements array and cleans up | |||
| // the non-stale index entries, safely skipping the stale object-index entry | |||
| // (it is undefined after mutation, so the `if (!this.index[p][h])` guard fires). | |||
| // | |||
| // Rebuilding a fresh Collection with the current elements ensures the new | |||
| // store entry is correctly indexed, preventing duplicate heads on re-fetch. | |||
| const currentElements = list.elements.slice() | |||
| const oldStatements = reverse | |||
| ? kb.statementsMatching(null, property, subject, dataDoc) | |||
| : kb.statementsMatching(subject, property, null, dataDoc) | |||
| oldStatements.forEach(st => kb.removeStatement(st)) | |||
| list = new $rdf.Collection(currentElements) | |||
| if (reverse) { | |||
| kb.add(list, property, subject, dataDoc) | |||
| } else { | |||
| kb.add(subject, property, list, dataDoc) | |||
| } | |||
| expect(getListHeads().length).toBe(2) | ||
|
|
||
| // After refresh, the field's internal list should be synced to one of the collections. | ||
| // The refresh function itself does not remove duplicates — that happens in saveListThenRefresh. | ||
| body.refresh!() | ||
|
|
||
| // Both heads still exist (removal happens during save) | ||
| expect(getListHeads().length).toBe(2) | ||
| }) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Ordered Multiple/Collection field does not serialize as proper RDF list issue #737
Should be resolved with rdflib v2.3.8