Skip to content

feat(ui-inputs): add GroupSelect and GroupCombobox atoms (v0.13.0) - #224

Merged
jasperboerhof merged 27 commits into
script-development:mainfrom
Elenyan-Script:feat/ui-inputs-group-select
Sep 23, 2026
Merged

jasperboerhof merged 27 commits into
script-development:mainfrom
Elenyan-Script:feat/ui-inputs-group-select

Conversation

@Elenyan-Script

@Elenyan-Script Elenyan-Script commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Adds GroupSelect (button trigger) and GroupCombobox (input trigger) atoms for grouped-option listboxes, mirroring the existing SingleSelect/Combobox and MultiSelect/MultiCombobox split
  • Introduces GroupRow union type (header | option) so group headers interleave with navigable options in a single row sequence without occupying index space
  • GroupOptionList.vue — internal listbox popup; headers carry role=presentation so they are valid listbox children but skipped by the keyboard path
  • WR-0576 browse-to-change equality rule, select-all-on-open, dismiss revert, async edit-form re-sync watch, and defineExpose focus handle (WR-0448) on GroupCombobox
  • 368 tests passing, 100% coverage

Test plan

  • npm run test:coverage passes with 100% coverage in packages/ui-inputs
  • GroupSelect: render/ARIA, open, group headers interleaved, select-on-click, clear entry, keyboard (ArrowDown/Enter/Escape/Home/End), aria-activedescendant, disabled, required/invalid/describedby, emptyText, header: false, mutedOptions, #option slot
  • GroupCombobox: render, open, filter narrowing with empty group hidden, WR-0576 browse-to-change, Escape revert, click-outside revert, select-all-on-open, keyboard commit, clear keyboard path, idle re-sync, async edit-form pattern, focus handle, #option slot

Generated with Claude Code

Two new atoms for grouped-option listboxes, mirroring the existing
SingleSelect/Combobox and MultiSelect/MultiCombobox split (ADR-0043):

- GroupRow union type (header | option) so group headers interleave with
  navigable options in a single row sequence without occupying index space
- GroupOptionList.vue — internal listbox popup; headers carry role=presentation
  so they are valid listbox children but are skipped by the keyboard path
- GroupSelect.vue — button trigger, non-searchable; flatOptions bridges all
  groups for index-keyed pointer, commit, and aria-selected
- GroupCombobox.vue — input trigger with per-group filtering; WR-0576
  browse-to-change equality rule, select-all-on-open, dismiss revert, and
  async edit-form re-sync watch; defineExpose focus handle (WR-0448)
- 46 tests (22 GroupSelect + 24 GroupCombobox), 100% coverage

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Elenyan-Script
Elenyan-Script requested a review from a team as a code owner August 28, 2026 11:41
- v8 ignore the unreachable race-guard `return false` in both commit()
  functions (defensive guard against a concurrent filter change between
  keydown and the watcher flush — cannot be triggered in tests)
- GroupSelect: add empty-group skip test (line 151 continue branch) and
  clear-entry mouseover test (GroupOptionList clearHover emit)
- GroupCombobox: add option mouseover test (@hover handler), getter label
  test (labelOf function branch), header=false test, and mutedOptions test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Goosterhof Goosterhof added the Agent Review Requested Requesting review of specialized AI review agents. label Aug 28, 2026

@jasperboerhof jasperboerhof 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.

Crit review

1 issue · 4 nitpicks · head 0c37abeb9a

Crit requests changes — 1 issue.

Issues

ui-inputs version bump for new public exports ships without a changeset file
packages/ui-inputs/package.json:3 — see inline

4 nitpicks

GroupSelect and GroupCombobox classes have no CSS rules in styles.css
packages/ui-inputs/styles.css:437 — GroupSelect.vue and GroupCombobox.vue apply ui-groupselect__* and ui-groupcombobox__* classes, but styles.css's shared menu/option/clear/empty rule blocks enumerate only the select, combobox, multiselect, and multicombobox variants. Neither new SFC carries its own style block. The popup menu, options, clear entry, empty row, and the new group-header row render with no background, padding, hover, or selection styling for these two components.

nitpick because pre-existing — this pull request did not write those lines

Group header v-for key is derived from display text, not group identity
packages/ui-inputs/src/components/GroupOptionList.vue:358 — GroupOptionList.vue keys header rows as h:${row.text} in its v-for loop instead of using a stable group identifier. Two groups sharing the same display text produce duplicate keys. On a re-render triggered by filtering, Vue's keyed diff can reuse or move the wrong header DOM node between the two same-named groups.

nitpick because pre-existing — this pull request did not write those lines; unconfirmed — proof gap: Requires exercising a live combobox with two same-text groups through a filter-triggered re-render and inspecting DOM node reuse to confirm a visible mis-render actually occurs.

axe-core browser audit suite omits the new GroupSelect and GroupCombobox components
packages/ui-inputs/tests/browser/axe.browser.spec.ts:17 — axe.browser.spec.ts audits every other listbox-family component — SingleSelect, Combobox, MultiSelect, MultiCombobox and others — but never imports or audits GroupSelect or GroupCombobox. GroupOptionList.vue reproduces the same role=option empty-row and clear-entry patterns the file's own comment says exist to be audited in a real browser. A real ARIA defect in the new grouped-listbox markup, such as an invalid aria-required-children combination, ships without this suite catching it.

nitpick because no runtime path — nothing reaches the harm at this head; pre-existing — this pull request did not write those lines

Real-Chromium interaction test suite omits GroupSelect and GroupCombobox
packages/ui-inputs/tests/browser/interaction.browser.spec.ts:15 — interaction.browser.spec.ts exercises real pointer and keyboard CDP events for every other listbox-family component but does not import or test GroupSelect or GroupCombobox. Native disabled-input click suppression, Popover API top-layer rendering, and floating-ui positioning under autoUpdate for the two new components are validated only by happy-dom's synthetic events. Any real-browser-only divergence in these behaviours would go undetected.

nitpick because no runtime path — nothing reaches the harm at this head; pre-existing — this pull request did not write those lines; unconfirmed — proof gap: Cannot run the Playwright/Chromium lane in this review to confirm an actual behavioural divergence between happy-dom and real-browser event handling for the two new components.

Comment thread packages/ui-inputs/package.json Outdated

@dmooibroek dmooibroek 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.

Adds GroupSelect/GroupCombobox atoms mirroring the existing select family, but ships zero CSS for the new class prefixes so the popup renders unstyled — plus group headers aren't conveyed to assistive tech and the option-list markup is forked rather than shared.

Not anchorable to the diff

  • [high] GroupSelect/GroupCombobox class families ship with zero CSS rules — New ui-groupselect/ui-groupcombobox classes (GroupSelect.vue, GroupCombobox.vue, GroupOptionList.vue) match no selector in styles.css — every existing rule enumerates only the four legacy families (.ui-select__menu, .ui-combobox__menu, .ui-multiselect__menu, .ui-multicombobox__menu :437-440, plus __option/__clear/__empty/chevron rules). styles.css is a published export ("./style.css": "./styles.css"), so any consumer importing it renders the popup unstyled: transparent/borderless/unscrollable, no option highlight, chevron falls back to 300x150 default, group-header indistinguishable from options. Fix: extend the existing shared selector groups with the two new prefixes, add a __group-header rule.

Comment thread packages/ui-inputs/src/components/GroupOptionList.vue Outdated
Comment thread packages/ui-inputs/src/components/GroupOptionList.vue Outdated

@jasperboerhof jasperboerhof 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.

Crit review

0 issues · 2 nitpicks · head 67187cb615

Crit requests changes — 1 open thread.

2 nitpicks

GroupSelect and GroupCombobox are never run through the real-browser axe-core audit
packages/ui-inputs/tests/browser/axe.browser.spec.ts — axe.browser.spec.ts mounts eleven existing components inside FormField and asserts zero axe violations in both open and closed states, but the new GroupSelect and GroupCombobox components are absent from its import list and from interaction.browser.spec.ts. These two components introduce a DOM shape none of the audited components use: role="presentation" header

  • rows interleaved with role="option" rows inside role="listbox", plus a new committing clear entry. Any real ARIA violation this shape produces, such as an aria-required-owned or aria-required-children failure, has no test that can catch it, since the happy-dom specs only assert attribute strings rather than running an accessibility-tree audit.

    nitpick because pre-existing — this pull request did not write those lines

    Group header row keys collide when two groups share the same header text
    packages/ui-inputs/src/components/GroupOptionList.vue:349 — GroupOptionList.vue keys header rows in its v-for as h:${row.text}, using only the group's display text with no group index or identity mixed in. The groups prop on both GroupSelect.vue and GroupCombobox.vue is typed as {options: T[]; text: string; header?: boolean}[] with no uniqueness requirement on text, and nothing in either component de-duplicates group text before it reaches GroupOptionList. When a caller passes two groups with identical text, such as two 'Other' buckets from server data, Vue receives duplicate keys in the same render pass, causing a duplicate-key warning and undefined vnode reuse between the two header nodes.

    nitpick because pre-existing — this pull request did not write those lines

  • Still open

    packages/ui-inputs/src/components/GroupOptionList.vue — already filed, still open


    Settled, not re-filed: 2

    Elenyan-Script and others added 3 commits August 28, 2026 15:17
    …eader ARIA
    
    Two issues raised in PR review:
    
    1. GroupSelect/GroupCombobox shipped with zero CSS rules — every existing shared
       selector block enumerated only the four legacy families. Extended menu, option,
       muted/active option, clear, clear-active, empty, and reduced-motion blocks with
       the two new prefixes. Added GroupSelect trigger/placeholder/chevron rules. Added
       group-container and group-header rules keyed on new --ui-group-header-* vars.
    
    2. Group headers used role="presentation", making group names invisible to AT. Changed
       GroupOptionList to the APG listbox grouping pattern: named groups render as
       <li role="group" aria-label="…"> with the visual header as an aria-hidden="true"
       span inside, so screen readers announce the group name without double-reading it.
       Options with header:false still render flat (no group wrapper). Updated two
       GroupSelect tests that asserted the old role="presentation" structure.
    
    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
    … coverage
    
    Tighten .ui-group{select,combobox}__group ul to a direct-child selector (> ul)
    so consumer <ul> elements inside the #option slot are not silently stripped of
    list-style/padding/margin.
    
    Add GroupSelect and GroupCombobox to axe.browser.spec.ts: three real-browser
    axe-core cases covering the grouped listbox DOM shape (role="group" + role="option"
    interleaved inside role="listbox"), the committing clear entry, and the filtered
    combobox path.
    
    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
    jasperboerhof
    jasperboerhof previously approved these changes Aug 28, 2026

    @jasperboerhof jasperboerhof 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.

    Crit review

    0 issues · 2 nitpicks · head bcd4c12f84

    Crit approves — nothing blocking at this head.

    2 nitpicks

    GroupSelect and GroupCombobox ship with no entry in the ui-inputs docs page
    docs/packages/ui-inputs.md:198 — The PR adds GroupSelect and GroupCombobox to packages/ui-inputs/src/index.ts, but docs/packages/ui-inputs.md is untouched. Its component table still lists only SingleSelect, Combobox, MultiSelect, and MultiCombobox from the select family. The line 'Two types complete the public surface' right after the table is now inaccurate. A consumer reading the canonical docs page has no way to discover the two new components or their props.

    nitpick because pre-existing — this pull request did not write those lines

    Browser-tests CI check status for the new grouped-listbox axe audits cannot be verified as green
    packages/ui-inputs/tests/browser/axe.browser.spec.ts:1635 — The diff adds three new axe-core audits for GroupSelect and GroupCombobox in the serially-run browser spec, exercising a role=group containing a role=presentation list of role=option items. Whether this new ARIA nesting passes axe's aria-required-children/aria-required-owned checks cannot be determined without running the browser test suite. If the new markup violates those rules, the accessibility regression would ship undetected by this review.

    nitpick because pre-existing — this pull request did not write those lines; unconfirmed — proof gap: No Bash/execution tool is available to run npm run test:browser (or vitest run against packages/ui-inputs/vitest.browser.config.ts) in this worktree to capture the actual pass/fail result and any violating assertion.


    Settled, not re-filed: 2

    …in GroupOptionList
    
    html-aria disallows role="group" on <li> (axe aria-allowed-role). The APG grouped
    listbox pattern is preserved by giving the wrapper <li> role="presentation" and
    moving role="group" + aria-label to the inner <ul>, which explicitly allows role="group".
    AT sees through the presentation hole to the named group as before.
    
    Update GroupSelect.spec.ts to assert the corrected structure.
    
    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

    @dmooibroek dmooibroek 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 adds GroupSelect/GroupCombobox atoms with grouped-option rendering; headline concern is a new grouping bug where a header:false group placed after a named group gets silently absorbed into that group's run (wrong aria-label, wrong visual block).

    Still open

    Full detail on each linked thread, not re-pasted here.

    • [medium] GroupOptionList forks OptionList markup instead of extending it — packages/ui-inputs/src/components/GroupOptionList.vue:1-94 — thread · change pushed but doesn't resolve — see reply

    Resolved since last review: 2.

    Comment thread packages/ui-inputs/src/components/GroupOptionList.vue Outdated

    @jasperboerhof jasperboerhof 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.

    Crit review

    0 issues · 1 nitpick · head 2fd8c4234a

    Crit requests changes — 1 open thread.

    1 nitpick

    GroupCombobox.spec.ts never exercises the required/invalid truthy branches, risking the 100% gate
    packages/ui-inputs/tests/GroupCombobox.spec.ts — GroupCombobox.vue binds :aria-required, :aria-invalid, and the is-invalid class to the required and invalid props, but GroupCombobox.spec.ts never mounts the component with either prop set to true. packages/ui-inputs/vitest.config.ts enforces branches:100 over src/**/*.{ts,vue}, and the sibling GroupSelect.spec.ts already covers the same three props in one test. Without an equivalent test, npm run test:coverage is at risk of failing on these newly-added truthy branches.

    nitpick because pre-existing — this pull request did not write those lines; unconfirmed — proof gap: Could not execute npm run test:coverage in this environment to confirm these specific branches are the ones that push ui-inputs coverage below the 100% threshold.

    Still open

    packages/ui-inputs/src/components/GroupOptionList.vue — already filed, still open


    Settled, not re-filed: 2

    @Goosterhof Goosterhof 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.

    One blocker anchored inline, plus confirmation on the prior round's findings.

    CI is red at this head (check/ci-passed fail the 100% coverage gate on packages/ui-inputs/src/**) — see the inline note on GroupOptionList.vue:52. The PR's own test-plan checkbox claiming 100% coverage doesn't hold at HEAD.

    Confirming the prior review's open items: the header:false-after-a-named-group absorption bug (GroupOptionList.vue:116) is real, traced through GroupSelect.vue's rows computed, which emits no boundary marker for a headerless run. The reuse-fork observation (GroupOptionList.vue vs OptionList.vue) also stands — the two are near-identical outside the grouping logic.

    The CSS and ARIA priors are fixed: styles.css now ships full .ui-groupselect/.ui-groupcombobox rules, and the group wrapper pairs role=group + aria-label with an aria-hidden visual header.

    Requesting changes on the coverage gate; the absorption bug and the reuse-fork note stand as before.

    Comment thread packages/ui-inputs/src/components/GroupOptionList.vue Outdated
    A `header:false` group that followed a named group had its options
    absorbed into the prior group's role="group": `groupedRuns` kept
    appending to the open run because a headerless group emitted no row to
    break it. Add a `boundary` GroupRow marker, emitted by both row builders
    (GroupSelect `rows` / GroupCombobox `filteredRows`) ahead of a headerless
    group's options; `groupedRuns` closes the open run on it so those options
    render flat via the existing `!current` path. Regression tests added to
    both spec files; 100% coverage held.
    
    Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

    @jasperboerhof jasperboerhof 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.

    Crit review

    1 issue · 0 nitpicks · head a7c4804f26

    Crit requests changes — 1 issue.

    Issues

    Document the exported grouped selects in the ui-inputs package guide
    docs/packages/ui-inputs.md:198 — not on the diff, so not inline

    The package barrel exports GroupSelect and GroupCombobox, but the guide lists only four select controls. The guide documents options as the required collection prop and never describes groups. Consumers cannot learn the grouped controls' required input shape from the published package documentation.


    Settled, not re-filed: 1

    …st forking OptionList
    
    GroupOptionList copied OptionList's option-row markup verbatim (role="option",
    aria-selected, is-active/is-muted chrome, hover/commit wiring, #option slot),
    diverging only in the group-run layout — so any future a11y/markup fix to an
    option had to land in two bodies.
    
    Extract that row into a new internal ListboxOption.vue and render it from both
    OptionList (flat) and GroupOptionList (grouped + headerless runs). The option
    markup now lives once; GroupOptionList owns only the role="group"/aria-label
    wrappers and the headerless-run layout. DOM is byte-identical; 100% coverage held
    (ListboxOption fully exercised via the existing suites).
    
    Addresses PR script-development#224 review finding (dmooibroek): GroupOptionList forks OptionList
    markup instead of extending it.
    
    Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

    @jasperboerhof jasperboerhof 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.

    Crit review

    1 issue · 1 nitpick · head 2a84089748

    Crit requests changes — 1 issue.

    Issues

    Public documentation omits the newly exported grouped select controls
    packages/ui-inputs/README.md:24 — not on the diff, so not inline

    The package exports GroupSelect and GroupCombobox. The README and package documentation catalog only the earlier select controls. Consumers cannot discover the grouped controls or their groups contract from published documentation.

    1 nitpick

    Published validator tests omit both grouped select components
    packages/ui-inputs/tests/published-prop-validators.spec.ts:41 — The published validator cases exclude GroupSelect and GroupCombobox. Their component tests import source SFCs. A distribution-only LabelKey validator regression can reach grouped-control consumers without CI detecting it.

    nitpick because code change required — the harm needs an edit that has not happened; pre-existing — this pull request did not write those lines

    dmooibroek
    dmooibroek previously approved these changes Sep 1, 2026

    @dmooibroek dmooibroek 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.

    Adds GroupSelect/GroupCombobox atoms with grouped-option rendering; both prior medium findings (absorption bug, forked markup) are fixed, but the fix itself reveals group-row derivation logic is now duplicated across the two components and should be extracted to the shared group-rows module.

    Resolved since last review: 2.

    Comment thread packages/ui-inputs/src/components/GroupSelect.vue Outdated
    …related components
    
    - Updated GroupSelect.vue to use OptionList for rendering options, improving consistency across components.
    - Modified MultiSelect, MultiCombobox, and SingleSelect to utilize rows instead of labels for options.
    - Enhanced OptionList to handle both flat and grouped options, collapsing group headers appropriately.
    - Adjusted tests for GroupSelect and GroupCombobox to reflect changes in option rendering and interaction.
    - Removed GroupOptionList component and updated related test utilities for clarity and maintainability.
    jasperboerhof
    jasperboerhof previously approved these changes Sep 1, 2026

    @jasperboerhof jasperboerhof 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.

    Crit review

    0 issues · 1 nitpick · head 19d0210e3d

    Crit approves — nothing blocking at this head.

    1 nitpick

    Published validator tests omit both grouped controls
    packages/ui-inputs/tests/published-prop-validators.spec.ts:39 — The CASES array omits GroupSelect and GroupCombobox despite both declaring label: LabelKey<T>.
    Only this suite imports the built package, as its documentation states.
    CI would not detect a dist validator regression for either grouped control.
    Consumers could receive prop-validation warnings.

    nitpick because code change required — the harm needs an edit that has not happened

    GroupSelect and GroupCombobox held the identical GroupRow-derivation loop
    (header/boundary + option-index pushing), so the header:false boundary fix
    had to land in both copies. Move the encoding to a single buildGroupRows()
    helper in src/internal/group-rows.ts and call it from both -- the GroupRow
    invariant is now single-site. The empty-group skip guard is a no-op on the
    GroupCombobox path (filteredData already drops empty groups). Behaviour and
    DOM unchanged; 100% coverage + browser suites held.
    
    Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

    @jasperboerhof jasperboerhof 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.

    Crit review

    0 issues · 0 nitpicks · head 99b69d150d

    Crit approves — nothing blocking at this head.

    No issues or nitpicks.

    dmooibroek
    dmooibroek previously approved these changes Sep 1, 2026

    @dmooibroek dmooibroek 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.

    Re-review: all 1 prior finding(s) resolved at this HEAD.

    Resolved since last review: 1.

    @crit-ai crit-ai left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Crit review

    0 issues · 1 nitpick · head e8af514dc1

    Crit approves — nothing blocking at this head.

    1 nitpick

    The ui-inputs mutation script skips the five new grouped listbox implementations
    packages/ui-inputs/package.json:43 — The test:mutation script only echoes a message. The root workspace command accepts that successful exit. Future behavior regressions can pass test:mutation without mutant detection.

    nitpick because code change required — the harm needs an edit that has not happened; pre-existing — this pull request did not write those lines


    Settled, not re-filed: 2

    @crit-ai

    crit-ai commented Sep 15, 2026

    Copy link
    Copy Markdown
    Collaborator

    Crit reviewt deze pull request. Crit is een automatische reviewer. Zijn review staat als aparte review op deze pull request, onder dit bericht of, bij een mislukte ronde, na de volgende poging.

    • Issue — blokkeert de pull request. Fix het en push, of zeg in de thread wat je laat staan en waarom.
    • Nitpick — blokkeert nooit en vraagt geen antwoord. Een nitpick laten liggen is een legitieme keuze.
    • Resolven — een thread zelf resolven verandert niets. Crit leest de code en je woorden. Eerst reageren, dan pushen.
    Een review lezen

    Eén review per ronde. Bovenaan de telling en het oordeel. Daaronder de issues, dan de nitpicks ingeklapt.

    • Een issue op een regel die in de diff zichtbaar is, staat ook inline op die regel.
    • Een issue elders staat in de samenvatting, gemarkeerd als not on the diff, so not inline.
    • Elke nitpick eindigt met de reden waarom hij geen issue is: nitpick because ….
    • Finder trace — onder elke issue staat een ingeklapt blok met de ruwe vondst: de claim, het gevolg, de paden die de zoeker volgde en de drie tags. Voor mensen optioneel; voor een agent die de fix maakt is dit het startpunt, want hier staan de paden die de zoeker las, niet alleen de verankerde regel.
    • Still open — een eerdere thread waarvan het probleem op deze head nog staat, met de alinea die zegt wat er nog ontbreekt en dezelfde finder trace.
    • Settled, not re-filed: N — vondsten die een eerdere thread al dekt; crit post ze niet opnieuw.
    Issue of nitpick

    Het verschil is niet ernst. Elke vondst draagt drie tags. Geldt één nitpick-waarde, dan is het een nitpick; anders een issue. Een vaste regel kiest, geen agent.

    tag waarde betekenis bucket
    harm_requires nothing het gaat nu al mis, zoals de code er staat issue
    harm_requires runtime_state het gaat mis onder een voorwaarde tijdens het draaien: een flag, data, timing issue
    harm_requires code_change het gaat pas mis na een latere wijziging die nog niet gedaan is nitpick
    harm_requires no_runtime_path geen pad bereikt het probleem: dode code, een onbereikbare tak nitpick
    provenance introduced deze pull request schreef de regel issue
    provenance adjacent ongewijzigde code die deze wijziging breekt of voedt issue
    provenance pre_existing stond er al en deze wijziging maakt het niet erger nitpick
    confidence confirmed crit volgde het pad; het klopt op deze head issue
    confidence unconfirmed een echte zorg die crit niet rond kreeg; de proof gap zegt wat ontbreekt nitpick
    • Runtime is een issue. Crit noemt het mechanisme, niet een kans; hoe vaak het optreedt weeg jij.
    • Buiten de diff is geen vrijstelling. Breekt jouw wijziging bestaande code, dan is dat adjacent en een issue.
    Wat doe je met een vondst

    Voor een issue werken drie dingen, zolang je het zegt.

    actie wat je doet wat crit doet
    Fixen fix, reageer in de thread, dán push ziet de nieuwe code en sluit de thread zelf
    Laten staan zeg wat je laat staan en wie het oppakt: "out of scope voor deze PR", "real, filed as KD-1341", "report aangemaakt in Kendo", "risico geaccepteerd" sluit de thread en blokkeert er niet meer op
    Weerleggen leg uit waarom het niet klopt, met iets dat crit kan nakijken: een pad, een test, een meting leest de code; klopt jouw uitleg, dan sluit crit de thread

    Wat niet werkt: een thread zelf resolven zonder fix of antwoord. "Werkt bij mij" en "fixen we later" tellen niet.

    Een nitpick laten liggen is een legitieme keuze. Hij blokkeert niet en vraagt geen antwoord. Oppakken, een ticket maken of niets doen: alle drie prima.

    oordeel wanneer
    request changes minstens één issue of één open thread
    approve geen van beide; nitpicks mogen blijven
    comment de repo heeft blokkeren of goedkeuren uit staan
    Veelgestelde vragen
    Een runtime-issue komt bij ons bijna nooit voor. Mag ik hem laten staan?

    Ja. Schrijf in de thread dat je het risico accepteert en waarom; crit sluit de thread. Let op: een retry, een cron-overlap of een dubbele webhook gebeurt ook bij één gebruiker.

    Crit vond iets in code die ik niet heb aangeraakt. Waarom staat dat op mijn pull request?

    Crit zet een vondst waar het probleem woont. Breekt jouw wijziging die code, dan is het adjacent en een issue; een oude bug los van jouw wijziging is pre_existing en een nitpick.

    Wat betekent "proof gap" en wat moet ik ermee?

    Crit zag een echt mechanisme maar kon één stap niet bewijzen; de proof gap zegt welke. Antwoord in de thread of dat pad bestaat, en fix het als dat zo is.

    Ik heb de thread geresolved op GitHub, maar crit komt er toch weer mee. Waarom?

    Crit kijkt naar de code, niet naar de knop. Staat het probleem op de nieuwe head nog, dan post crit het opnieuw; fix het of schrijf in de thread waarom je het laat staan.

    Wat moet ik precies in een thread schrijven om iets te laten staan?

    Noem het gedrag dat je laat staan en wie het oppakt; een Kendo-report zonder ticketnummer telt ook. Crit zoekt het report nooit op en leest alleen wat jij in de thread schrijft.

    Ik heb gefixt en gepusht, maar crit ziet mijn reactie niet. Wat ging er mis?

    Crit leest de threads vlak na een push, dus een reactie van daarna mist die ronde. Eerst reageren, dan pushen; de fix zelf ziet crit altijd.

    Mag ik alles in de finder trace vertrouwen?

    De paden en regels wel: crit liep ze na voordat de issue werd gepost. De zinnen eromheen niet altijd. De vetgedrukte kop en de alinea eronder zijn de geverifieerde tekst; waar de trace daarvan afwijkt, wint de alinea. Een absolute bijzin in de trace ("only called by tests") is een claim van de zoeker, geen oordeel.

    Ik laat een agent de fix maken. Wat geef ik hem?

    De hele comment, inclusief de trace. De trace kan verdere sites noemen; laat hem elke site in de repo controleren voordat hij fixt. Een issue die "three routes" zegt en er één verankert, noemt de andere twee in de trace. Een pad onder node_modules of vendor is leesspoor, geen site.

    Moet ik op nitpicks reageren?

    Nee. Laten liggen is een legitieme keuze: een nitpick heeft geen thread en blokkeert niet. Zolang de code hetzelfde blijft, kan hij in een volgende ronde opnieuw in de ingeklapte lijst staan.

    Waarom zegt crit "request changes" terwijl er geen nieuwe issues zijn?

    Een eerdere thread waarvan het probleem nog staat, blokkeert ook. Kijk onder Still open: daar staat de thread met wat er nog ontbreekt.

    Kan crit per repo minder streng?

    Ja, met twee schakelaars per repo: approve en request changes. Staat request changes uit, dan wordt het oordeel comment en blokkeert de pull request nooit.

    @crit-ai crit-ai left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Crit review

    2 issues · 1 nitpick · head 16bda20139

    Crit requests changes — 2 issues.

    Issues

    Group-header aliases prevent scoped option-token overrides from reaching headers
    packages/ui-inputs/styles.css:107 — see inline

    The theming inventory omits the new group-header custom-property surface
    docs/packages/ui-inputs.md:440 — not on the diff, so not inline

    The stylesheet declares six --ui-group-header-* tokens. The documented inventory has no group-header category. Consumers cannot discover the supported group-header overrides through the documented contract.

    Sites
    docs/packages/ui-inputs.md:436-452
    packages/ui-inputs/styles.css:104-112
    packages/ui-inputs/styles.css:624-629

    Finder trace · general lane · not verified by the judge beyond the text above

    Claim. The theming documentation omits all six newly introduced --ui-group-header-* variables from its variable-surface inventory.

    Consequence. Consumers relying on the documented theming contract cannot discover the supported group-header padding, color, size, weight, transform, or tracking overrides; the inventory is incomplete for the new public component surface.

    Evidence. The diff introduces --ui-group-header-pad, --ui-group-header-color, --ui-group-header-size, --ui-group-header-weight, --ui-group-header-transform, and --ui-group-header-tracking at packages/ui-inputs/styles.css:104-112 and uses them at :624-629. The documented variable categories at docs/packages/ui-inputs.md:440-450 list Field, Pressable, Control, Listbox, Option, Chip, Check, Switch, and Error surfaces but no Group header category, while the page describes that list as the variable surface and says the stylesheet is authoritative at :452.

    Precedent. docs/packages/ui-inputs.md:446 — the Option category enumerates its complete public token surface, establishing the inventory convention used for the other component families.

    harm_requires: nothing · provenance: adjacent · confidence: confirmed

    1 nitpick

    Published validator coverage omits GroupSelect and GroupCombobox
    packages/ui-inputs/tests/published-prop-validators.spec.ts:41 — The built-package validator table lists six components and omits both grouped exports. GroupSelect and GroupCombobox source tests import their SFCs directly. Packaged validators can regress without failing grouped component tests.

    Sites
    packages/ui-inputs/tests/published-prop-validators.spec.ts:20-25
    packages/ui-inputs/tests/published-prop-validators.spec.ts:39-48
    packages/ui-inputs/tests/GroupSelect.spec.ts:6-34
    packages/ui-inputs/tests/GroupCombobox.spec.ts:6-34
    packages/ui-inputs/src/index.ts:16-17

    nitpick because code change required — the harm needs an edit that has not happened; pre-existing — this pull request did not write those lines


    Settled, not re-filed: 1

    Comment thread packages/ui-inputs/styles.css

    @crit-ai crit-ai left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Crit review

    1 issue · 1 nitpick · head 16bda20139

    Crit requests changes — 1 issue, 1 open thread.

    Issues

    Sparse arrays create option rows whose indices do not exist in flatOptions.
    packages/ui-inputs/src/internal/group-rows.ts:32 — see inline

    1 nitpick

    Published validator census omits the two newly exported grouped select components.
    packages/ui-inputs/tests/published-prop-validators.spec.ts:41 — CASES lists only six older components despite exporting GroupSelect and GroupCombobox. Source SFC tests cannot observe tsdown's emitted prop validators. A validator regression can therefore reach consumers using label property names.

    Sites
    packages/ui-inputs/tests/published-prop-validators.spec.ts:20-29
    packages/ui-inputs/tests/published-prop-validators.spec.ts:39-48
    packages/ui-inputs/src/index.ts:16-17
    packages/ui-inputs/src/components/GroupSelect.vue:109-113
    packages/ui-inputs/src/components/GroupCombobox.vue:101-105

    nitpick because code change required — the harm needs an edit that has not happened; pre-existing — this pull request did not write those lines

    Still open

    packages/ui-inputs/styles.css — already filed, still open

    Comment thread packages/ui-inputs/src/internal/group-rows.ts Outdated
    Elenyan-Script and others added 2 commits September 15, 2026 11:07
    Adds the six --ui-group-header-* custom properties (GroupSelect /
    GroupCombobox) to the theming inventory in docs/packages/ui-inputs.md,
    alongside the Option category. The tokens already ship and are wired in
    styles.css; the inventory just omitted the new group-header category so
    consumers could not discover the overrides through the documented contract.
    
    Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
    buildGroupRows counted options with `for...of`, which visits array holes
    (yielding undefined), while both grouped parents derive `flatOptions` via
    `groups.flatMap((g) => g.options)`, which skips holes. A sparse but
    type-valid T[] therefore emitted option rows whose indices did not exist
    in flatOptions, so OptionList dereferenced flatOptions[index].id and the
    popup threw during render. A sparse groups array hit the same divergence
    one level up.
    
    Switch both loops to forEach (hole-skipping, exactly like flatMap's
    flatten) and defer a group's header/boundary row until it has >=1 real
    option row, so the row count is definitionally identical to the flattened
    option list on either vector. Adds a sparse-array render test.
    
    Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

    @crit-ai crit-ai left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Crit review

    1 issue · 0 nitpicks · head db2281da3f

    Crit requests changes — 1 issue.

    Issues

    OptionList dereferences holes in flat option rows before flat controls render options
    packages/ui-inputs/src/components/OptionList.vue:190 — see inline


    Settled, not re-filed: 1

    Comment thread packages/ui-inputs/src/components/OptionList.vue
    crit-ai
    crit-ai previously approved these changes Sep 15, 2026

    @crit-ai crit-ai left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Crit review

    0 issues · 1 nitpick · head db2281da3f

    Crit approves — nothing blocking at this head.

    1 nitpick

    Published-artifact tests omit GroupSelect and GroupCombobox label validation
    packages/ui-inputs/src/index.ts:16 — The built-package CASES list excludes GroupSelect and GroupCombobox. Both components expose their display-string prop as label: LabelKey<T>. Source-importing component tests cannot check the compiled validators. Consumers can receive a bad built export or label validator without this suite detecting it.

    Sites
    packages/ui-inputs/tests/published-prop-validators.spec.ts:2-45
    packages/ui-inputs/src/components/GroupSelect.vue:110-113
    packages/ui-inputs/src/components/GroupCombobox.vue:102-105

    nitpick because code change required — the harm needs an edit that has not happened


    Settled, not re-filed: 1

    auto-merge was automatically disabled September 18, 2026 09:16

    Head branch was pushed to by a user without write access

    @crit-ai crit-ai left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Crit review

    0 issues · 2 nitpicks · head c3dbeac452

    Crit approves — nothing blocking at this head.

    2 nitpicks

    Published LabelKey validation omits the two grouped select components
    packages/ui-inputs/tests/published-prop-validators.spec.ts:41 — published-prop-validators.spec.ts imports the built package but excludes GroupSelect and GroupCombobox from CASES. Their source-level specifications import the SFCs directly. A dist-only validator regression can therefore warn when consumers pass a string label.

    Sites
    packages/ui-inputs/tests/published-prop-validators.spec.ts:41-48

    nitpick because code change required — the harm needs an edit that has not happened

    Top-layer assertions exclude the two grouped select components
    packages/ui-inputs/tests/listbox-top-layer.spec.ts:18 — listbox-top-layer.spec.ts limits FAMILY to the four older select components. GroupSelect and GroupCombobox create their own popover anchors. Their component specifications only assert that an opened menu renders. A grouped anchor can lose top-layer promotion without a test detecting it.

    Sites
    packages/ui-inputs/tests/listbox-top-layer.spec.ts:18-23
    packages/ui-inputs/tests/GroupSelect.spec.ts:54-57
    packages/ui-inputs/tests/GroupCombobox.spec.ts:53-61

    nitpick because code change required — the harm needs an edit that has not happened


    Settled, not re-filed: 1

    @Goosterhof Goosterhof 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.

    Re-review at c3dbeac4. My 2026-09-08 blocker is cleared: packages/ui-inputs/package.json reads 0.14.0, main still holds 0.13.0 (and so does the npm registry) — no version collision, this bump will actually publish. docs/, README.md and styles.css all reflect the two new atoms.

    Checked the one commit since I'd expect suspicion, 550fc681 ("update configuration files and tests across multiple packages") — it touches only axe.browser.spec.ts, and it strengthens the harness rather than weakening any assertion: it wraps the rendered FormField in <main>/<h1> so axe's page-level rules (landmark-one-main, page-has-heading-one, region) stop firing against the bare test page and actually audit the component. No rule is disabled.

    Non-blocking nit: the PR title still says (v0.13.0) against a 0.14.0 bump — cosmetic, author's call.

    crit-ai's two nitpicks at this head (LabelKey validator + top-layer spec both skip the two new grouped components) are real coverage gaps but non-blocking by crit's own tiering — leaving them to the author.

    CI green (browser-tests/check/ci-passed), no open issues from any reviewer at this head. Approved.

    prsweep:c3dbeac452c924c6528d36b3ce561172fd68ae19

    @jasperboerhof
    jasperboerhof merged commit ab9f260 into script-development:main Sep 23, 2026
    3 checks passed

    @crit-ai crit-ai left a comment

    Copy link
    Copy Markdown
    Collaborator

    Choose a reason for hiding this comment

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

    Crit review

    0 issues · 0 nitpicks · head 3c8a72445d

    Crit approves — nothing blocking at this head.

    No issues or nitpicks.


    Settled, not re-filed: 2

    Goosterhof added a commit that referenced this pull request Sep 28, 2026
    Absorbs 28 base commits (#224 GroupSelect / GroupCombobox, shared
    ListboxOption, group-rows.ts, ui-inputs 0.14.0).
    
    One conflict, packages/ui-inputs/tests/browser/axe.browser.spec.ts:
    main added FRUIT_GROUPS beside FieldSlot, which the branch had turned
    from an object `type` into an `interface`. Both kept: main's fixture,
    the branch's interface.
    
    Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
    Claude-Session: https://claude.ai/code/session_01TEtr4wU9UTG6scb35qh74T
    Goosterhof added a commit that referenced this pull request Sep 28, 2026
    … point (WR-1633)
    
    #224's GroupSelect / GroupCombobox specs arrived after the rule landed and
    declared `type Fruit = {id: number; name: string}` — the only two fires of
    the six rules on the merged tree (consistent-type-definitions). Converted to
    the interface shape the sibling specs already use. Test-local; nothing
    published changes.
    
    Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
    Claude-Session: https://claude.ai/code/session_01TEtr4wU9UTG6scb35qh74T
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Agent Review Requested Requesting review of specialized AI review agents.

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    5 participants