Skip to content

Replace CategoryOptions expanded mode VAutocomplete with KMultiSelect - #6355

Open
sarwar-jahin-port wants to merge 5 commits into
learningequality:unstablefrom
sarwar-jahin-port:refactor/kmultiselect-category-options-6346
Open

sarwar-jahin-port wants to merge 5 commits into
learningequality:unstablefrom
sarwar-jahin-port:refactor/kmultiselect-category-options-6346

Conversation

@sarwar-jahin-port

@sarwar-jahin-port sarwar-jahin-port commented Oct 28, 2026 •

Copy link
Copy Markdown

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 expanded prop 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.

onKMultiSelectInput initially 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 the cleared message callback's undocumented timing was safe. abhiraj75 then traced it through EditBooleanMapModal.handleSave and found the carryover wasn't actually load-bearing: when categories start mixed across the edited nodes, handleSave rebuilds each node's saved categories from currentNode[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. So onKMultiSelectInput is back to the same one-line rebuild dropdown mode already used, identical for both modes now, with no carryover, no cleared side effect, and no dependency on undocumented KDS behavior.

A separate nit (abhiraj75): listMaxHeight was 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:

  • Dropdown mode (Channel editor → select one resource → Edit → Details tab → Category): search/filter, select, parent/child indentation, chip + tooltip, individual chip removal, clear-all — all unchanged.
  • Expanded mode (Channel editor → select resource(s) → options menu → Edit categories): list renders open with no dropdown chrome, search filters it, chips + tooltips work, individual removal and clear-all work, keyboard navigation works.
  • Full save/reload round trip confirmed via the Vuex store (not just the UI) after each manual test, to rule out the editor's autosave silently writing a stale/empty selection.
  • Clear-all empties the field correctly; existing EditBooleanMapModal.spec.js coverage (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-utils suite (EditBooleanMapModal.spec.js) broke because it looked up options by a data-test attribute the old checkbox markup had and KMultiSelect doesn't render; its lookup helpers are updated to query KMultiSelect's KListboxOption instances instead — same tests, same coverage, no behavior assertions changed.

pnpm test and pre-commit run --all-files pass.

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 inEditBooleanMapModal.spec.js so the full suite stays green, scoped to only what this change broke.

@sarwar-jahin-port
sarwar-jahin-port requested review from a team and leq-bot October 28, 2026 18:50
@learning-equality-bot

Copy link
Copy Markdown
Contributor

👋 Hi @sarwar-jahin-port, thanks for contributing!

For the review process to begin, please verify that the following is satisfied:

  • Contribution is aligned with our contributing guidelines

  • Pull request description has correctly filled AI usage section & follows our AI guidance:

    AI guidance

    State explicitly whether you didn't use or used AI & how.

    If you used it, ensure that the PR is aligned with Using AI as well as our DEEP framework. DEEP asks you:

    • Disclose — Be open about when you've used AI for support.
    • Engage critically — Question what is generated. Review code for correctness and unnecessary complexity.
    • Edit — Review and refine AI output. Remove unnecessary code and verify it still works after your edits.
    • Process sharing — Explain how you used the AI so others can learn.

    Examples of good disclosures:

    "I used Claude Code to implement the component, prompting it to follow the pattern in ComponentX. I reviewed the generated code, removed unnecessary error handling, and verified the tests pass."

    "I brainstormed the approach with Gemini, then had it write failing tests for the feature. After reviewing the tests, I used Claude Code to generate the implementation. I refactored the output to reduce verbosity and ran the full test suite."

📢✨ Before we assign a reviewer:

  • @rtibblesbot will pre-review this pull request. Its comments are generated by an LLM, and should be evaluated accordingly.
  • We'll also invite community pre-review. See the community review guidance for both authors and reviewers.

Also check that issue requirements are satisfied & you ran pre-commit locally.

Pull requests that don't follow the guidelines will be closed.

Reviewer assignment can take up to 2 weeks.

@rtibblesbot

rtibblesbot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Waiting for changes

Outcome: skipped — no new commits since 214d6b7

Last updated: 2026-11-01 19:57 UTC

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rtibblesbot
rtibblesbot dismissed their stale review October 28, 2026 19:19

Superseded by my review of b40bd7b: no blocking findings remain.

@rtibblesbot rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cleared message 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 rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rtibblesbot
rtibblesbot dismissed their stale review October 29, 2026 13:42

Superseded by my review of f343577: no blocking findings remain.

@abhiraj75 abhiraj75 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rtibblesbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Remove Vuetify from Studio] Replace CategoryOptions VAutocomplete with KMultiSelect expanded mode

3 participants