Migrate CategoryOptions dropdown to KMultiSelect - #6092
Conversation
|
👋 Hi @Prashant-thakur77, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
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. |
|
@LianaHarris360 I’ve created the PR for the hierarchical use of KMultiSelect in CategoryOptions |
4416411 to
11094ae
Compare
|
📢✨ Before we assign a reviewer, we'll turn on |
🟡 Waiting for changesLast updated: 2026-08-19 20:53 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6092 migrates the CategoryOptions dropdown to KMultiSelect. The mechanics are right: level from getSortedCategories() is depth-first, which is what useMultiSelectCascade requires, and messages supplies every key the KMultiSelect/KListbox validators demand. One blocking i18n gap on the chip remove button; the rest is dead code the migration left behind, plus test gaps.
CI passing. Manual QA was required for this PR but did not run — no UI was visually verified.
- blocking: chip close button ships an untranslated English accessible name
- suggestion: unreachable partial-carryover branch, now-dead VAutocomplete slots, ten strings hoisted to
commonStringsfor one consumer, hard-coded translated text in tests, no test for either headline behaviour change - nitpick:
setup()boilerplate, redundant:multiple, assertion style
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| <template #chip="{ option, remove: removeChip }"> | ||
| <span :ref="'category-chip-' + option.value"> | ||
| <KChip | ||
| :text="option.text" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: KChip gets :text and close but no removeLabel, so its close button falls back to KDS's hard-coded English (KChip/index.vue:107 — `Remove ${this.text}`). Every category chip's ✕ is announced as Remove Dance in every locale, and categoryOptions.spec.js:98 asserts that literal, pinning it in place.
Fix is one binding plus one new string. Since the KTooltip is trigger="hover", the full path is mouse-only today — passing it here covers keyboard and screen reader users too:
:removeLabel="removeCategoryAction$({ label: tooltipText(option.value) })"| <KChip | ||
| :text="option.text" | ||
| close | ||
| @close="removeChip" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: KDS's default chip slot puts @mousedown.native.prevent on the KChip (KMultiSelectInput.vue:30); this override drops it. Without it, mousedown on the close button blurs the input, so onInputBlur schedules the rAF at KMultiSelect/index.vue:433 that can closeDropdown() and emit blur. onChipRemove refocuses on nextTick and normally wins the race — was chip removal checked with the dropdown open? Adding the modifier removes the question.
| zIndex: 4, | ||
| height: expanded ? 0 : 'auto', | ||
| maxHeight: expanded ? 0 : 300, | ||
| height: 0, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: With the VAutocomplete now behind v-if="expanded" and its menu pinned to height: 0 / maxHeight: 0, nothing inside the menu can render again. Unreachable now: #item (lines 89-105, with treeItemStyle/isSelected/onChange), #no-data (79-87), and the .parentOption rule at line 380 whose only user is line 92. The expanded branch still needs only #selection and :searchInput.sync.
There was a problem hiding this comment.
done,removed
| // Rebuilds the { category: [nodeIds] } object from KMultiSelect's flat | ||
| // array. Categories not applied to every edited node are invisible to | ||
| // KMultiSelect (see autocompleteValues), so they are carried over untouched. | ||
| onKMultiSelectInput(newValues) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The ids.length !== this.nodeIds.length branch is unreachable. Dropdown mode renders only from DetailsTabView.vue:133 under v-if="oneSelected" (nodes.length === 1), and generateNestedNodesGetterSetterObject pushes one node id per node; the other call site (EditCategoriesModal) passes expanded. A hypothetical partial wouldn't survive the round trip anyway — the parent setter discards them (DetailsTabView.vue:601).
Suggest reducing to the reachable case and reinstating the carryover with the expanded-mode migration that actually needs it:
this.selected = Object.fromEntries(newValues.map(value => [value, this.nodeIds]));| components: { DropdownWrapper }, | ||
| components: { KMultiSelect, KChip, DropdownWrapper }, | ||
| mixins: [constantsTranslationMixin, metadataTranslationMixin], | ||
| setup() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: 26 lines of setup() to forward ten functions that are only read inside messages. SidePanelModal/index.vue:195 destructures commonStrings inside the computed that needs it, which collapses this to a couple of lines.
| message: 'Options', | ||
| context: 'Tooltip for the generic options menu icon', | ||
| }, | ||
| clearAllAction: { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: These ten are KMultiSelect plumbing with exactly one consumer — KMultiSelect appears nowhere else in the frontend — which is below the bar for hoisting into the shared module.
The move is also incomplete: communityChannelsStrings.clearAllAction already exists with the identical 'Clear all' and is what NotificationList.vue:113 imports, so the next author has two clearAllAction$s to guess between. Either keep these in CategoryOptions' own $trs until a second adopter lands, or repoint NotificationList.vue at the common one and delete the duplicate.
There was a problem hiding this comment.
Let's reuse the existing string from communityChannelsStrings. It belongs in the common file but can't be moved right now because that would break the existing translation reference, so please add this comment above its placement in communityChannelsStrings.js:
// TODO: clearAllAction should be moved to commonStrings.js in next major Studio release
| const DANCE = 'd&WXdXWF.5QAjgfv7.BUMJJBnS'; | ||
| const MUSIC = 'd&WXdXWF.5QAjgfv7.u0aKjT4i'; | ||
|
|
||
| const SCHOOL_LABEL = 'School'; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: These labels, plus 'Category' (35), `Remove ${DANCE_LABEL}` (98), 'Clear all' (106) and 'Open menu' (114), all render through a translator and are all importable — the category labels via translateMetadataString(camelCase(...)) (EditBooleanMapModal.spec.js:21 does exactly this), the rest from the commonStrings this PR just added (CollapsibleToolbar.spec.js:5-7). As written the suite re-asserts the English copy rather than the wiring: a wording change breaks it, a missing translation doesn't.
| }); | ||
| }); | ||
|
|
||
| it('preserves partially applied categories when the selection changes', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: No call site produces nodeIds: [NODE_1, NODE_2] in dropdown mode (see the comment on onKMultiSelectInput), so this pins forward-compat behaviour rather than shipped behaviour — it will keep passing regardless of whether the eventual bulk-edit migration is correct.
| const item = 'abcd'; | ||
| wrapper.vm.$emit = jest.fn(); | ||
| wrapper.vm.add(item); | ||
| it('removes a category when its chip close button is clicked', async () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Neither behaviour change the PR calls out has a dropdown-mode test:
- indeterminate parents: with
value: { [DANCE]: [NODE_1] }, clickingSchoolshould now emit{ [DANCE]: [NODE_1], [SCHOOL]: [NODE_1] }rather than clearing the branch. - descendant removal, now KDS's level cascade instead of Studio's prefix matching: removing the
Artschip with{ [ARTS]: [NODE_1], [DANCE]: [NODE_1] }should emit{}— the dropdown twin of line 128.
The second is what would catch a regression in the level depth-first contract getDescendants relies on.
| }); | ||
| // The closed dropdown stays in the DOM (v-show), so queries must not look inside it. | ||
| const chipsArea = within(screen.getByRole('group')); | ||
| expect(chipsArea.getAllByText(DANCE_LABEL).length).toBeGreaterThan(0); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: getAllByText already throws when nothing matches, so .length).toBeGreaterThan(0) can't fail on its own — getByText(DANCE_LABEL) asserts the same thing and additionally catches a duplicated chip. Same shape at line 115. Separately, renders the flat checkbox list ... in expanded mode (111) sits in the dropdown mode block rather than the expanded mode one below it.
There was a problem hiding this comment.
KChip renders two nested elements with the same text, so getByText throws a “found multiple elements” error while i was checking
So for now I kept getAllByText and added a comment explaining why.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6092 — all 11 prior findings resolved or settled; no new findings in 11094ae..dcaabe2.
CI passing. Not approving only because the manual QA pass did not run — nothing in this delta was visually verified.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:20 — KChip close button fell back to KDS hard-coded English
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:23 — chip slot dropped KDS's @mousedown.native.prevent
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:53 — unreachable #item / #no-data / .parentOption behind the pinned-shut menu
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:242 — unreachable partial-carryover branch in onKMultiSelectInput
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:149 — 26-line setup() forwarding strings used only in messages
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/CategoryOptions.vue:11 — redundant :multiple="true"
RESOLVED — contentcuration/contentcuration/frontend/shared/strings/commonStrings.js:49 — duplicate clearAllAction$ / premature hoist
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/tests/categoryOptions.spec.js:11 — English copy asserted instead of translated strings
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/tests/categoryOptions.spec.js:80 — two-node dropdown case no call site produces
RESOLVED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/tests/categoryOptions.spec.js:105 — dropdown-mode tests for both called-out behaviour changes
ACKNOWLEDGED — contentcuration/contentcuration/frontend/shared/views/contentNodeFields/tests/categoryOptions.spec.js:59 — getAllByText stands; rebuttal accepted (chip text matches twice)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
@LianaHarris360 I have the changes required after review:) |
3 participants
Summary
Migrates the
CategoryOptionsdropdown from Vuetify'sVAutocompleteto KDSKMultiSelect, as part of moving Studio off Vuetify. This is the Category field in a resource's Edit details panel.The expanded mode used by the bulk edit modal is intentionally left on
VAutocompleteit needs an inline mode and externally controlled indeterminate values from KMultiSelect, which don't exist yet.Partof: learningequality/kolibri-design-system#1259
Notable points:
onKMultiSelectInputconverts KMultiSelect's flat array back to{ categoryId: [nodeIds] }and preserves partially applied categories. Partials can't occur today (single node only): this is forward-compatibility for the expanded-mode migration.References
Screencast.From.2026-08-17.02-11-04.mp4
Expanded Mode:
Screencast.From.2026-08-17.02-20-34.mp4
Reviewer guidance
All tests are covered and there are no regressions.
AI usage
I used Claude to generate this discription and done changes as needed