Repository navigation
Replace CategoryOptions expanded mode VAutocomplete with KMultiSelect - #6355
sarwar-jahin-port wants to merge 5 commits into
Conversation
|
👋 Hi @sarwar-jahin-port, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
📢✨ Before we assign a reviewer:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
🟡 Waiting for changesOutcome: skipped — no new commits since Last updated: 2026-11-01 19:57 UTC |
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6355: in multi-node editing, clear-all keeps partially applied categories. Otherwise expanded mode moves cleanly to KMultiSelect.
CI passes. Manual QA did not run, so the UI is unverified. I could not check the KDS expanded/listMaxHeight props without node_modules.
- blocking: clear-all keeps partially applied categories (inline)
- suggestion: removing a parent no longer drops its selected children (inline)
Next step: Needs a full code review by a human engineer, then manual QA once that reviewer sets Needs QA.
- The author is outside the team: confirm this should be built at all.
- User-visible files changed:
contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue. - Decide whether removing a parent category should also remove its selected children, as the old expanded mode did.
- In the multi-node category edit modal, check the KMultiSelect expanded list: its 250px height, filtering, and clearing partial selections.
Written by rtibblesbot, an LLM-based coding agent.
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6355: both prior findings are resolved. One new suggestion is inline.
CI passing. Manual QA did not run.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:122 — Clear-all no longer clears partially applied categories
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:117 — Removing a parent chip no longer removes its selected children
Next step: Needs a full code review by a human engineer, then manual QA once that reviewer sets Needs QA.
- The author is outside the team: confirm this should be built at all.
- User-visible files changed:
contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue. - Check that clear-all, removing a parent chip, and removing a single chip handle partially applied categories correctly in the bulk-edit category field.
Written by rtibblesbot, an LLM-based coding agent.
Superseded by my review of b40bd7b: no blocking findings remain.
…tInput when val is empty.
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6355: removing the last full chip now drops every partial category (blocking, see inline). Please fix it as the inline comment asks.
2 of 3 prior findings are resolved. I withdraw the third, because its fix caused this regression.
CI passing. Manual QA did not run.
Prior-finding status
RESOLVED — CategoryOptions.vue:123 — Clear-all no longer clears partially applied categories
RESOLVED — CategoryOptions.vue:114 — Removing a parent chip no longer removes its selected children
RESOLVED — CategoryOptions.vue — Clear-all depends on undocumented KDS behaviour (withdrawn: its fix caused the new blocking finding)
Next step: Needs a full code review by a human engineer, then manual QA once that reviewer sets Needs QA.
- The author is outside the team: confirm this should be built at all.
- User-visible files changed:
contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue. - Decide whether resetting state inside the
clearedmessage getter is acceptable until KDS exposes a clear-all event. - In the edit modal with several resources, check that clear-all, removing the last chip, and unchecking the last row each keep or clear partial categories correctly.
Written by rtibblesbot, an LLM-based coding agent.
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6355: all 4 prior findings are resolved or acknowledged. No new findings. Manual QA did not run.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:126 — Clear-all no longer clears partially applied categories
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:121 — Removing a parent chip no longer removes its selected children
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:113 — Clear-all depends on undocumented KDS behaviour (withdrawn: conflicts with the last-chip fix)
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:121 — Removing the last full chip drops every partial category
Next step: Needs a full code review by a human engineer, then manual QA once that reviewer sets Needs QA.
- The author is outside the team: confirm this should be built at all.
- User-visible files changed:
contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue. - In the bulk-edit category picker, check that Clear-all removes both full and partial categories, and that removing the last full chip keeps the unrelated partial ones.
Written by rtibblesbot, an LLM-based coding agent.
Superseded by my review of f343577: no blocking findings remain.
abhiraj75
left a comment
There was a problem hiding this comment.
Hey @sarwar-jahin-port! I'm Abhiraj, I added the expanded mode to KMultiSelect in KDS (learningequality/kolibri-design-system#1327), so it's great to see it used here. Really nice cleanup, dropping the old checkbox list logic makes this much easier to follow. Left a question and a small nit inline. Ran categoryOptions.spec.js and EditBooleanMapModal.spec.js locally and they pass. Thanks!
| // array from KMultiSelect's `input` event, so the two can't be told apart | ||
| // there. `cleared` is the only callback KMultiSelect invokes specifically | ||
| // for Clear-all, so it's the one place this reset can live safely. | ||
| cleared: () => { |
There was a problem hiding this comment.
I was trying to understand when a partial category actually matters here. In EditBooleanMapModal.handleSave, if the resources have mixed categories it starts from each node's existing categories and only adds the full ones (the modal also says "This will not remove existing categories", and there's a spec for it: "should not remove common selected options even if they are unchecked"). Without mixed categories I don't think a partial entry can exist. So does keeping partials, and the reset here in cleared, change what gets saved? If not, maybe we don't need to rely on cleared being called after input?
If it does turn out to be needed, I'm happy to look at adding a proper clear event to KMultiSelect on the KDS side so this doesn't have to live in cleared.
Also noticed Clear-all now emits input twice ({} and then {} again, in both modes), so in DetailsTabView update runs twice. Probably harmless, just mentioning.
There was a problem hiding this comment.
I traced it through handleSave and you're correct. When hasMixedCategories, it rebuilds fieldValue from currentNode[this.field] read straight from the store, and only layers in the fully-applied entries from CategoryOptions. It never reads back the partial ones. So the carryover/removedIds logic and the cleared hack weren't protecting anything real. Reverted onKMultiSelectInput to the plain one-line rebuild, same as dropdown mode — no carryover, no cleared side effect, no dependency on undocumented KMultiSelect timing. Appreciate you digging into the actual save path instead of taking my word for the component-level behavior being correct in isolation.
| itemText="text" | ||
| :label="translateMetadataString('category')" | ||
| :autoPromoteParent="false" | ||
| :listMaxHeight="expanded ? '250px' : undefined" |
There was a problem hiding this comment.
Small nit: KDS defaults listMaxHeight to 256px, so maybe this ternary isn't needed? Also the old list had a fixed height: 250px, now it's a max-height so the modal gets shorter while filtering. Not sure if that matters.
There was a problem hiding this comment.
yeah, dead weight removed it entirely, both modes now just use KMultiSelect's own 256px default. And good catch on the fixed-vs-max-height difference; I'm fine leaving it as max-height (shrinks while filtering) rather than fighting the component for a fixed box, since that's how KMultiSelect's expanded mode is meant to behave everywhere else it's used. Flag if you feel differently.
There was a problem hiding this comment.
Thanks @sarwar-jahin-port, looks good to me! Agree on leaving it as max-height, that's how expanded mode is meant to behave.
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6355: all 4 prior findings resolved.
Withdrawn: my ask to keep partial categories through clear-all and chip removal. EditBooleanMapModal.handleSave already re-applies them, so that carryover was redundant.
CI passing. Manual QA did not run.
Prior-finding status
RESOLVED — CategoryOptions.vue — Clear-all no longer clears partially applied categories (withdrawn)
RESOLVED — CategoryOptions.vue:113 — Removing a parent chip no longer removes its selected children
RESOLVED — CategoryOptions.vue — Clear-all depends on undocumented KDS behaviour
RESOLVED — CategoryOptions.vue — Removing the last full chip drops every partial category (withdrawn)
Next step: Needs a full code review by a human engineer, then manual QA once that reviewer sets Needs QA.
- The author is outside the team: confirm this should be built at all.
- User-visible files changed:
contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue. - In the quick-edit categories modal with mixed selections across nodes, check that clear-all, chip removal and save leave each node's partially applied categories intact.
- Check that the expanded-mode KMultiSelect shows and toggles parent/child categories as the old picker did.
Written by rtibblesbot, an LLM-based coding agent.
Summary
CategoryOptions had two modes: a dropdown (already migrated to KDS's KMultiSelect in #6092) and an expanded mode, used by the bulk "Edit categories" modal, still built on Vuetify's VAutocomplete plus a hand-rolled checkbox list. This PR finishes that migration: both modes now render a single KMultiSelect, using its new
expandedprop for the always-open listbox. VAutocomplete, DropdownWrapper, VTooltip and VChip are fully removed from the file.The two modes now share one category tree, one selected-values model, and one set of strings, as called for by the issue. This let me delete the bespoke multi-node selection logic (
isSelected,isCheckboxSelectedByChildren,add,remove,onChange,treeItemStyle,filteredCategories) in favor of KMultiSelect's own filtering, hierarchical indentation and indeterminate-parent display.One intentional, pre-existing behavior change carries over from #6092: a parent category whose children are all selected now shows as indeterminate rather than checked, and clicking it adds just the parent instead of clearing the whole branch. This was already shipped for dropdown mode; expanded mode now matches it instead of diverging.
onKMultiSelectInputinitially carried over categories not applied to every edited node, reinstating a branch #6092's reviewer had stripped out as unreachable until expanded mode needed it. Review (rtibblesbot, abhiraj75) went through several rounds on this: whether Clear-all correctly cleared partial categories, whether removing a parent chip cascaded to a partially-applied descendant, and whether relying on theclearedmessage callback's undocumented timing was safe. abhiraj75 then traced it throughEditBooleanMapModal.handleSaveand found the carryover wasn't actually load-bearing: when categories start mixed across the edited nodes,handleSaverebuilds each node's saved categories fromcurrentNode[field]read directly from the store, and only layers CategoryOptions' fully-applied selections on top — it never reads back the partial entries CategoryOptions was carrying. SoonKMultiSelectInputis back to the same one-line rebuild dropdown mode already used, identical for both modes now, with no carryover, noclearedside effect, and no dependency on undocumented KDS behavior.A separate nit (abhiraj75):
listMaxHeightwas set to'250px'for expanded mode and left unset for dropdown mode, but KMultiSelect already defaults to'256px', so both branches landed on essentially the same thing. Removed.References
Reviewer guidance
Manual verification performed locally:
EditBooleanMapModal.spec.jscoverage (e.g. "should not remove common selected options even if they are unchecked") already verifies that mixed-category saves are protected independently of what CategoryOptions emits.categoryOptions.spec.js's expanded-mode tests are rewritten against KMultiSelect's actual rendered DOM and extended to cover the indeterminate-parent click case, and now assert the simplified contract directly: a category only partially applied to the edited nodes is not carried over across an unrelated add, remove, or clear.A separate, pre-existing
@vue/test-utilssuite (EditBooleanMapModal.spec.js) broke because it looked up options by adata-testattribute the old checkbox markup had and KMultiSelect doesn't render; its lookup helpers are updated to query KMultiSelect'sKListboxOptioninstances instead — same tests, same coverage, no behavior assertions changed.pnpm testandpre-commit run --all-filespass.AI usage
I used Claude Code for this change. It read the existing CategoryOptions.vue, researched the issue, the linked prior PR (#6092) and its review thread, and the KDS documentation to work out the intended approach before writing any
code, then implemented the template/script changes and the test rewrite. I reviewed the diff, had it run the full test suite and pre-commit, and had it manually QA both modes in a local running instance. I also had it fix the resulting regression in
EditBooleanMapModal.spec.jsso the full suite stays green, scoped to only what this change broke.