refactor(components): DLT-3160 standardize v-model event handling#1201
Merged
Brad Paugh (braddialpad) merged 6 commits intonextfrom Apr 17, 2026
Merged
refactor(components): DLT-3160 standardize v-model event handling#1201Brad Paugh (braddialpad) merged 6 commits intonextfrom
Brad Paugh (braddialpad) merged 6 commits intonextfrom
Conversation
Brad Paugh (braddialpad)
requested review from
Francis Rupert (francisrupert),
Ignacio Ropolo (iropolo) and
Nina Repetto (ninarepetto)
as code owners
Contributor
|
Please add either the |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced legacy native Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Brad Paugh (braddialpad)
added
the
no-visual-test
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a71870f12f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/dialtone-vue/components/tooltip/tooltip.vue (1)
216-223:⚠️ Potential issue | 🟡 MinorRemove stale
.syncwording from theopenprop docs.The prop now documents
v-model:opencontrol, but the.syncnote is still present and conflicts with current Vue 3 guidance.✏️ Suggested doc fix
/** * Controls whether the tooltip is shown. Leaving this null will have the tooltip trigger on mouseover by default. * If you set this value, the default mouseover behavior will be disabled and you can control it as you need. - * Supports .sync modifier + * Use v-model:open for controlled visibility. * `@values` null, true, false */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/tooltip/tooltip.vue` around lines 216 - 223, Update the docs for the tooltip component's open prop to remove the stale ".sync" wording and reference the Vue 3 v-model syntax instead: edit the JSDoc for the open prop in tooltip.vue (the open prop definition) to remove the "Supports .sync modifier" line and replace it with a note that control is via "v-model:open" (or controlled externally via Boolean/null), keeping the type and default unchanged.packages/dialtone-vue/components/select_menu/select_menu.vue (1)
267-283:⚠️ Potential issue | 🟠 MajorAdd BREAKING CHANGE footer to commit; breaking change confirmed in event API.
The
inputevent was removed from the component's emits (previously:'input','update:modelValue','change'; now:'update:modelValue','change'). Tests and stories have been updated to reflect this change. However, the commit message lacks aBREAKING CHANGE:footer required by the guidelines for breaking API changes in public packages. For a public npm library, this breaking change must include:BREAKING CHANGE: input event removed; use update:modelValueThis ensures the package version bumps to MAJOR and notifies consumers of the migration path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/select_menu/select_menu.vue` around lines 267 - 283, The commit for the change in select_menu.vue removed the 'input' emit from the component emits (now only 'update:modelValue' and 'change' are emitted), which is a breaking API change; update the commit message to include a BREAKING CHANGE footer exactly stating: "BREAKING CHANGE: input event removed; use update:modelValue" so consumers are informed and the package will be versioned as a MAJOR release.packages/dialtone-vue/components/radio_group/radio_group.vue (1)
48-56:⚠️ Potential issue | 🟠 MajorMark the
inputevent removal as breaking.This drops a public event from a published component. Consumers still using
@inputor legacyv-modelwiring will stop updating after release, so this needs explicit BREAKING CHANGE release metadata and migration notes.As per coding guidelines "
packages/dialtone-vue/components/**: ... Flag any prop/event/slot removal or rename without BREAKING CHANGE footer."Also applies to: 87-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/radio_group/radio_group.vue` around lines 48 - 56, You removed the public "input" event from the radio_group component (emits array in radio_group.vue and related emits at lines ~87-89), which is a breaking API change for consumers using `@input` or legacy v-model; update the release to include a BREAKING CHANGE footer noting removal of the "input" event, add migration notes instructing consumers to switch to "update:modelValue" or use v-model:value semantics, and update any component docs/examples and changelog entries to reference radio_group.vue and the removed "input" event so downstream users can adapt.packages/dialtone-vue/components/checkbox_group/checkbox_group.vue (1)
20-25:⚠️ Potential issue | 🟠 MajorTreat the
selectedValues→modelValuecontract swap as breaking.This renames the public v-model prop and event pair from
selectedValues/update:selectedValuestomodelValue/update:modelValue. Existing consumers will break silently unless the release is explicitly marked as BREAKING CHANGE and the migration path is called out.As per coding guidelines "
packages/dialtone-vue/components/**: ... Flag any prop/event/slot removal or rename without BREAKING CHANGE footer."Also applies to: 52-60, 91-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/checkbox_group/checkbox_group.vue` around lines 20 - 25, This change renames the public v-model prop/event from selectedValues/update:selectedValues to modelValue/update:modelValue which is a breaking API change; restore backward compatibility by supporting both names in the component (accept both props and emit both events), keep the new modelValue as primary, map selectedValues to the same reactive value, and emit update:selectedValues alongside update:modelValue (and vice‑versa for incoming values) while logging a deprecation warning when selectedValues is used; update the relevant symbols: the prop definitions (modelValue / selectedValues) and emit points (update:modelValue / update:selectedValues) in the CheckboxGroup component to implement this compatibility shim.packages/dialtone-vue/components/radio_group/radio_group.test.js (1)
184-197:⚠️ Potential issue | 🟠 MajorUse the mounted instance for emitted-event assertion.
Line 197 asserts emissions on
wrapper, but the interaction was triggered onmountWrapper. This can mask regressions.🛠️ Suggested fix
- expect(wrapper.emitted('update:modelValue')).toBeFalsy(); + expect(mountWrapper.emitted('update:modelValue')).toBeFalsy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/radio_group/radio_group.test.js` around lines 184 - 197, The test is asserting emissions on the wrong instance: after mounting DtRadioGroup into mountWrapper and triggering change on mountedRadioGroup, replace the assertion that checks wrapper.emitted('update:modelValue') with an assertion against mountWrapper.emitted('update:modelValue') (i.e., use the mounted component instance created by mount(DtRadioGroup) rather than the unrelated wrapper variable) so the test correctly inspects events emitted by DtRadioGroup after mountedRadioGroup.trigger('change').packages/dialtone-vue/components/input/input.test.js (1)
661-674:⚠️ Potential issue | 🟠 MajorWire listener and remove duplicate assertions.
The listener setup does not match the event assertions. Non-textarea case uses
onInput, but test checksupdate:modelValue. Textarea case wires no listener at all, yet asserts the callback was called. Both need'onUpdate:modelValue': MOCK_INPUT_STUBwired. Additionally, tests contain duplicate consecutive assertions (e.g., lines 750–751, 760–761, 773–774, 821–824) that violate the "one assertion per test" guideline.🛠️ Suggested fix
describe('When type is not a textarea', () => { beforeEach(() => { mockProps = { currentLength: null, validate: MOCK_VALIDATE }; - mockAttrs = { onInput: MOCK_INPUT_STUB }; + mockAttrs = { 'onUpdate:modelValue': MOCK_INPUT_STUB }; updateWrapper(); nativeInput.setValue(MOCK_USER_TEXT_INPUT_VAL); }); it('should handle input value', () => { expect(wrapper.emitted()['update:modelValue'][0][0]).toEqual(MOCK_USER_TEXT_INPUT_VAL); expect(MOCK_INPUT_STUB).toHaveBeenCalled(); });describe('When type is a textarea', () => { beforeEach(() => { mockProps = { type: 'textarea', currentLength: null, validate: MOCK_VALIDATE }; + mockAttrs = { 'onUpdate:modelValue': MOCK_INPUT_STUB }; updateWrapper(); nativeTextarea.setValue(MOCK_USER_TEXT_INPUT_VAL); }); it('should handle input value', () => { expect(wrapper.emitted()['update:modelValue'][0][0]).toEqual(MOCK_USER_TEXT_INPUT_VAL); expect(MOCK_INPUT_STUB).toHaveBeenCalled(); });For duplicate assertions, consolidate into single assertions per test (lines 750–751, 760–761, 773–774, 821–824).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/input/input.test.js` around lines 661 - 674, The tests for non-textarea and textarea are wiring the wrong listener and contain duplicate assertions; update the test setup so both cases pass 'onUpdate:modelValue': MOCK_INPUT_STUB via mockAttrs (instead of only onInput or none) before calling updateWrapper()/nativeInput.setValue, and remove or consolidate duplicate consecutive expect(...) calls into a single assertion per test (references: updateWrapper, mockAttrs, mockProps, nativeInput, wrapper, MOCK_INPUT_STUB, MOCK_USER_TEXT_INPUT_VAL).
🧹 Nitpick comments (4)
packages/dialtone-vue/components/checkbox/checkbox.test.js (2)
420-428: Prefer emitted-event behavior assertion over handler call count.Line 427 checks spy invocation count; this is more implementation-coupled than asserting emitted
update:modelValuebehavior from user interaction.As per coding guidelines
packages/dialtone-vue/**/*.test.js: "Flag tests that assert on implementation details (mock call counts) instead of behavior (rendered output, emitted events)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/checkbox/checkbox.test.js` around lines 420 - 428, Replace the implementation-coupled assertion that checks MOCK_INPUT_LISTENER_SPY invocation with a behavioral assertion on the component's emitted event: after setting mockAttrs (with MOCK_INPUT_LISTENER_SPY) and calling updateWrapper() then awaiting input.trigger('change'), assert that the wrapper emitted an 'update:modelValue' event (use wrapper.emitted('update:modelValue') or equivalent) and validate its payload/count instead of using toHaveBeenCalledTimes on MOCK_INPUT_LISTENER_SPY.
316-393: Consolidate duplicated custom-event suites.Lines 316-353 and Lines 356-393 test the same three
update:modelValuescenarios. Please merge into one shared suite (e.g.,it.each) to reduce drift and maintenance noise.As per coding guidelines
packages/dialtone-vue/**/*.test.js: "Use beforeAll/beforeEach to reduce repetition. Use it.each for similar tests with different values."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/checkbox/checkbox.test.js` around lines 316 - 393, There are two identical "Custom Event Tests" suites testing the same update:modelValue scenarios; remove the duplicate block and consolidate into a single suite (e.g., under the shared describe for Accessibility/Interactivity or higher) that uses beforeEach to set up mockProps and call updateWrapper and it.each to run the three cases: default (no props) -> trigger('change') -> expect(wrapper.emitted('update:modelValue')).toBeTruthy(), checked (mockProps = { modelValue: true }) -> updateWrapper -> trigger('change') -> expect emitted truthy, and disabled (mockProps = { disabled: true }) -> updateWrapper -> trigger('click') -> expect emitted falsy; reference wrapper, input, mockProps, updateWrapper, and wrapper.emitted('update:modelValue') in the new consolidated tests.packages/dialtone-vue/components/input/input.vue (1)
389-391:update:modelValueJSDoc type is incomplete for file inputs.Line 390 documents
String | Number, but Lines 473-476 emitstring[]whentype="file". Please include array payload in the event type docs.Proposed doc fix
- * `@type` {String | Number} + * `@type` {String | Number | String[]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/input/input.vue` around lines 389 - 391, Update the JSDoc for the component event "update:modelValue" in input.vue to include the file-input payload type; change the documented type from "String | Number" to "String | Number | String[]" (or equivalent) so it matches the emit behavior in the component where type="file" emits a string[] payload.packages/dialtone-vue/components/input/input.test.js (1)
750-751: Remove duplicated assertions in IME tests.Several tests now assert the exact same
update:modelValueexpectation twice. Keep one assertion per condition to avoid redundant checks and noisy failures.Also applies to: 760-761, 773-776, 800-801, 810-811, 821-824
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/input/input.test.js` around lines 750 - 751, Tests contain duplicated assertions checking wrapper.emitted()['update:modelValue'] (e.g., the two identical expect(...) lines shown) — remove the duplicate expect in each occurrence so each IME condition asserts update:modelValue only once; update the affected blocks that reference wrapper.emitted()['update:modelValue'] (also at the other noted ranges) to leave a single expect per condition and run the test suite to verify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dialtone-vue/components/checkbox_group/checkbox_group.stories.js`:
- Around line 19-22: This change renames the checkbox-group public API to use
modelValue / update:modelValue (see modelValue and 'onUpdate:modelValue' in
checkbox_group.stories and related props/events), which is a public breaking
change; update the PR/commit message to include an explicit "BREAKING CHANGE:"
footer describing the API rename/removal (mention old prop/event name and the
new modelValue/update:modelValue) so release tooling will treat this as a major
change and consumers are notified.
In `@packages/dialtone-vue/components/checkbox_group/checkbox_group.test.js`:
- Around line 219-226: The test calls the async helper
MOCK_SELECTED_CHECKBOX_FUNCTION(MOCK_SELECTED_VALUE) without awaiting it,
causing the assertion on wrapper.emitted('update:modelValue') to run too early;
make the test function async (the "it('does not emit update:modelValue event',
...)" block) and await MOCK_SELECTED_CHECKBOX_FUNCTION(MOCK_SELECTED_VALUE)
after calling updateWrapper() so the interaction completes before asserting on
wrapper.emitted.
In `@packages/dialtone-vue/components/toggle/toggle.vue`:
- Around line 143-147: The JSDoc for the emitted event update:modelValue is
incorrect: it lists "Boolean | String" while the component emits a Boolean value
(uses !this.internalChecked). Update the JSDoc above the component to reflect
the actual runtime type (Boolean) and, if the component ever accepts string-like
values, adjust the emit logic in the toggle component (internalChecked and the
code path that emits update:modelValue) to ensure types are consistent;
otherwise change the `@type` to Boolean so the documentation matches the emitted
value.
---
Outside diff comments:
In `@packages/dialtone-vue/components/checkbox_group/checkbox_group.vue`:
- Around line 20-25: This change renames the public v-model prop/event from
selectedValues/update:selectedValues to modelValue/update:modelValue which is a
breaking API change; restore backward compatibility by supporting both names in
the component (accept both props and emit both events), keep the new modelValue
as primary, map selectedValues to the same reactive value, and emit
update:selectedValues alongside update:modelValue (and vice‑versa for incoming
values) while logging a deprecation warning when selectedValues is used; update
the relevant symbols: the prop definitions (modelValue / selectedValues) and
emit points (update:modelValue / update:selectedValues) in the CheckboxGroup
component to implement this compatibility shim.
In `@packages/dialtone-vue/components/input/input.test.js`:
- Around line 661-674: The tests for non-textarea and textarea are wiring the
wrong listener and contain duplicate assertions; update the test setup so both
cases pass 'onUpdate:modelValue': MOCK_INPUT_STUB via mockAttrs (instead of only
onInput or none) before calling updateWrapper()/nativeInput.setValue, and remove
or consolidate duplicate consecutive expect(...) calls into a single assertion
per test (references: updateWrapper, mockAttrs, mockProps, nativeInput, wrapper,
MOCK_INPUT_STUB, MOCK_USER_TEXT_INPUT_VAL).
In `@packages/dialtone-vue/components/radio_group/radio_group.test.js`:
- Around line 184-197: The test is asserting emissions on the wrong instance:
after mounting DtRadioGroup into mountWrapper and triggering change on
mountedRadioGroup, replace the assertion that checks
wrapper.emitted('update:modelValue') with an assertion against
mountWrapper.emitted('update:modelValue') (i.e., use the mounted component
instance created by mount(DtRadioGroup) rather than the unrelated wrapper
variable) so the test correctly inspects events emitted by DtRadioGroup after
mountedRadioGroup.trigger('change').
In `@packages/dialtone-vue/components/radio_group/radio_group.vue`:
- Around line 48-56: You removed the public "input" event from the radio_group
component (emits array in radio_group.vue and related emits at lines ~87-89),
which is a breaking API change for consumers using `@input` or legacy v-model;
update the release to include a BREAKING CHANGE footer noting removal of the
"input" event, add migration notes instructing consumers to switch to
"update:modelValue" or use v-model:value semantics, and update any component
docs/examples and changelog entries to reference radio_group.vue and the removed
"input" event so downstream users can adapt.
In `@packages/dialtone-vue/components/select_menu/select_menu.vue`:
- Around line 267-283: The commit for the change in select_menu.vue removed the
'input' emit from the component emits (now only 'update:modelValue' and 'change'
are emitted), which is a breaking API change; update the commit message to
include a BREAKING CHANGE footer exactly stating: "BREAKING CHANGE: input event
removed; use update:modelValue" so consumers are informed and the package will
be versioned as a MAJOR release.
In `@packages/dialtone-vue/components/tooltip/tooltip.vue`:
- Around line 216-223: Update the docs for the tooltip component's open prop to
remove the stale ".sync" wording and reference the Vue 3 v-model syntax instead:
edit the JSDoc for the open prop in tooltip.vue (the open prop definition) to
remove the "Supports .sync modifier" line and replace it with a note that
control is via "v-model:open" (or controlled externally via Boolean/null),
keeping the type and default unchanged.
---
Nitpick comments:
In `@packages/dialtone-vue/components/checkbox/checkbox.test.js`:
- Around line 420-428: Replace the implementation-coupled assertion that checks
MOCK_INPUT_LISTENER_SPY invocation with a behavioral assertion on the
component's emitted event: after setting mockAttrs (with
MOCK_INPUT_LISTENER_SPY) and calling updateWrapper() then awaiting
input.trigger('change'), assert that the wrapper emitted an 'update:modelValue'
event (use wrapper.emitted('update:modelValue') or equivalent) and validate its
payload/count instead of using toHaveBeenCalledTimes on MOCK_INPUT_LISTENER_SPY.
- Around line 316-393: There are two identical "Custom Event Tests" suites
testing the same update:modelValue scenarios; remove the duplicate block and
consolidate into a single suite (e.g., under the shared describe for
Accessibility/Interactivity or higher) that uses beforeEach to set up mockProps
and call updateWrapper and it.each to run the three cases: default (no props) ->
trigger('change') -> expect(wrapper.emitted('update:modelValue')).toBeTruthy(),
checked (mockProps = { modelValue: true }) -> updateWrapper -> trigger('change')
-> expect emitted truthy, and disabled (mockProps = { disabled: true }) ->
updateWrapper -> trigger('click') -> expect emitted falsy; reference wrapper,
input, mockProps, updateWrapper, and wrapper.emitted('update:modelValue') in the
new consolidated tests.
In `@packages/dialtone-vue/components/input/input.test.js`:
- Around line 750-751: Tests contain duplicated assertions checking
wrapper.emitted()['update:modelValue'] (e.g., the two identical expect(...)
lines shown) — remove the duplicate expect in each occurrence so each IME
condition asserts update:modelValue only once; update the affected blocks that
reference wrapper.emitted()['update:modelValue'] (also at the other noted
ranges) to leave a single expect per condition and run the test suite to verify.
In `@packages/dialtone-vue/components/input/input.vue`:
- Around line 389-391: Update the JSDoc for the component event
"update:modelValue" in input.vue to include the file-input payload type; change
the documented type from "String | Number" to "String | Number | String[]" (or
equivalent) so it matches the emit behavior in the component where type="file"
emits a string[] payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d7d7b142-b5b1-4e78-8492-e0d22c8b232f
📒 Files selected for processing (40)
packages/dialtone-vue/components/checkbox/checkbox.stories.jspackages/dialtone-vue/components/checkbox/checkbox.test.jspackages/dialtone-vue/components/checkbox/checkbox.vuepackages/dialtone-vue/components/checkbox/checkbox_default.story.vuepackages/dialtone-vue/components/checkbox_group/checkbox_group.stories.jspackages/dialtone-vue/components/checkbox_group/checkbox_group.test.jspackages/dialtone-vue/components/checkbox_group/checkbox_group.vuepackages/dialtone-vue/components/checkbox_group/checkbox_group_default.story.vuepackages/dialtone-vue/components/collapsible/collapsible.vuepackages/dialtone-vue/components/dropdown/dropdown.vuepackages/dialtone-vue/components/filter_pill/filter_pill.vuepackages/dialtone-vue/components/filter_pill/filter_pill_variants.story.vuepackages/dialtone-vue/components/image_viewer/image_viewer.vuepackages/dialtone-vue/components/input/input.stories.jspackages/dialtone-vue/components/input/input.test.jspackages/dialtone-vue/components/input/input.vuepackages/dialtone-vue/components/input/input_default.story.vuepackages/dialtone-vue/components/input/input_search_variant.story.vuepackages/dialtone-vue/components/input_group/decorators/input.vuepackages/dialtone-vue/components/input_group/input_group.stories.jspackages/dialtone-vue/components/input_group/input_group_default.story.vuepackages/dialtone-vue/components/popover/popover.vuepackages/dialtone-vue/components/radio/radio.stories.jspackages/dialtone-vue/components/radio/radio.test.jspackages/dialtone-vue/components/radio/radio.vuepackages/dialtone-vue/components/radio/radio_default.story.vuepackages/dialtone-vue/components/radio_group/radio_group.stories.jspackages/dialtone-vue/components/radio_group/radio_group.test.jspackages/dialtone-vue/components/radio_group/radio_group.vuepackages/dialtone-vue/components/radio_group/radio_group_default.story.vuepackages/dialtone-vue/components/rich_text_editor/rich_text_editor.stories.jspackages/dialtone-vue/components/rich_text_editor/rich_text_editor.test.jspackages/dialtone-vue/components/rich_text_editor/rich_text_editor.vuepackages/dialtone-vue/components/rich_text_editor/rich_text_editor_default.story.vuepackages/dialtone-vue/components/select_menu/select_menu.stories.jspackages/dialtone-vue/components/select_menu/select_menu.test.jspackages/dialtone-vue/components/select_menu/select_menu.vuepackages/dialtone-vue/components/select_menu/select_menu_default.story.vuepackages/dialtone-vue/components/toggle/toggle.vuepackages/dialtone-vue/components/tooltip/tooltip.vue
💤 Files with no reviewable changes (3)
- packages/dialtone-vue/components/checkbox/checkbox.vue
- packages/dialtone-vue/components/input_group/decorators/input.vue
- packages/dialtone-vue/components/filter_pill/filter_pill_variants.story.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dialtone-vue/components/combobox_multi_select/combobox_multi_select.vue (1)
465-470:⚠️ Potential issue | 🔴 Critical
onInputlistener is mismatched for DtInput's actual event emission.DtInput only emits
update:modelValue, notinput. At line 468, the listeneronInput: event => { this.$emit('update:modelValue', event); }will never fire because DtInput doesn't emit that event. Similarly, the template binding@input="onInput"(line 67) won't work. Without this fix, the component cannot listen to input changes and won't emitupdate:modelValue.Fix
computed: { inputListeners () { return { ...extractVueListeners(this.$attrs), - onInput: event => { - this.$emit('update:modelValue', event); + 'onUpdate:modelValue': value => { + this.$emit('update:modelValue', value); if (this.hasSuggestionList) { this.showComboboxList(); } },- `@input`="onInput" + `@update`:model-value="onInput"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/combobox_multi_select/combobox_multi_select.vue` around lines 465 - 470, The onInput handler in inputListeners is wrong for DtInput because DtInput emits update:modelValue, not input; replace the onInput mapping and the template `@input`="onInput" usage with an onUpdate:modelValue handler so the component reacts to DtInput emissions — update the inputListeners() return to include onUpdate:modelValue: value => { this.$emit('update:modelValue', value); if (this.hasSuggestionList) { ... } } (and remove or rename the onInput reference in the template) so emitted model updates are captured correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dialtone-documentation/docs/components/toast.md`:
- Line 21: The DtCheckbox is emitting the new checked boolean via
update:modelValue but the current toggleImportant handler ignores that value and
flips state; update the handler used on DtCheckbox (or replace it) so it accepts
the emitted boolean and assigns it directly to the bound state (e.g., set the
local reactive/refs variable to the emitted value) instead of toggling, ensuring
the component state reflects the emitted value from DtCheckbox.
In `@packages/dialtone-vue/common/mixins/input_group.js`:
- Around line 123-127: The JSDoc for the emitted event 'update:modelValue'
incorrectly declares the payload as KeyboardEvent; update both occurrences (the
JSDoc blocks around lines referencing 'update:modelValue' in input_group.js) to
reflect the actual payload (the emitted newValue union type) instead of
KeyboardEvent, i.e., change the `@type` to the proper value union for newValue so
the docs match the emitted payload.
- Around line 121-129: The mixin now emits 'update:modelValue' while still
exposing the value prop, so either declare an explicit model mapping on the
component that uses this mixin (set the model option to map prop named "value"
to the event named "update:modelValue") to preserve standard v-model behavior,
or if you intend to change the public API, update the commit message to include
a BREAKING CHANGE footer that clearly states the emitted event was renamed to
"update:modelValue" and consumers must update their v-model usage accordingly.
In `@packages/dialtone-vue/components/rich_text_editor/rich_text_editor.vue`:
- Around line 480-487: The change removed the legacy public event 'input' from
the rich_text_editor component (emits array) which is a breaking API change; add
a clear BREAKING CHANGE footer to the commit/changelog/release notes stating:
"BREAKING CHANGE: removed 'input' event from rich_text_editor — consumers must
use 'update:modelValue' instead," and include a short migration note and
release-note entry so the removal doesn't ship silently as a patch.
---
Outside diff comments:
In
`@packages/dialtone-vue/components/combobox_multi_select/combobox_multi_select.vue`:
- Around line 465-470: The onInput handler in inputListeners is wrong for
DtInput because DtInput emits update:modelValue, not input; replace the onInput
mapping and the template `@input`="onInput" usage with an onUpdate:modelValue
handler so the component reacts to DtInput emissions — update the
inputListeners() return to include onUpdate:modelValue: value => {
this.$emit('update:modelValue', value); if (this.hasSuggestionList) { ... } }
(and remove or rename the onInput reference in the template) so emitted model
updates are captured correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 33b143a0-2ac9-440e-945e-b3d098d380f7
📒 Files selected for processing (26)
apps/dialtone-documentation/docs/components/banner.mdapps/dialtone-documentation/docs/components/combobox-multi-select.mdapps/dialtone-documentation/docs/components/combobox-with-popover.mdapps/dialtone-documentation/docs/components/filter-pill.mdapps/dialtone-documentation/docs/components/input-group.mdapps/dialtone-documentation/docs/components/radio-group.mdapps/dialtone-documentation/docs/components/select-menu.mdapps/dialtone-documentation/docs/components/toast.mdapps/dialtone-documentation/docs/components/toggle.mdpackages/dialtone-vue/common/mixins/input_group.jspackages/dialtone-vue/components/combobox_multi_select/combobox_multi_select.vuepackages/dialtone-vue/components/combobox_multi_select/combobox_multi_select_default.story.vuepackages/dialtone-vue/components/input_group/input_group.test.jspackages/dialtone-vue/components/radio/radio.test.jspackages/dialtone-vue/components/rich_text_editor/rich_text_editor.vuepackages/dialtone-vue/components/select_menu/select_menu.test.jspackages/dialtone-vue/components/select_menu/select_menu.vuepackages/dialtone-vue/components/select_menu/select_menu_default.story.vuepackages/dialtone-vue/components/toggle/toggle.vuepackages/dialtone-vue/components/toggle/toggle_default.story.vuepackages/dialtone-vue/recipes/conversation_view/editor/editor.vuepackages/dialtone-vue/recipes/conversation_view/editor/editor_default.story.vuepackages/dialtone-vue/recipes/conversation_view/editor/editor_signature.story.vuepackages/dialtone-vue/recipes/conversation_view/message_input/message_input.vuepackages/dialtone-vue/recipes/conversation_view/message_input/message_input_default.story.vuepackages/dialtone-vue/recipes/conversation_view/message_input/message_input_markdown_output.story.vue
✅ Files skipped from review due to trivial changes (2)
- apps/dialtone-documentation/docs/components/toggle.md
- apps/dialtone-documentation/docs/components/banner.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/dialtone-vue/components/select_menu/select_menu.test.js
- packages/dialtone-vue/components/radio/radio.test.js
- packages/dialtone-vue/components/select_menu/select_menu_default.story.vue
- packages/dialtone-vue/components/select_menu/select_menu.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dialtone-vue/components/toggle/toggle.vue (1)
79-81:⚠️ Potential issue | 🟡 MinorUpdate
toggleOnClickJSDoc to reflect the current event contract.This text still says “Change events will still be triggered,” but the component now emits
update:modelValue; the wording is stale and can mislead consumers.Proposed doc update
- * Whether the component toggles on click. If you set this to false it means you will handle the toggling manually - * via the checked prop or v-model. Change events will still be triggered. + * Whether the component toggles on click. If set to false, parent code must control state via modelValue/v-model. + * User interaction still emits update:modelValue.As per coding guidelines:
packages/dialtone-vue/components/**: Events:update:modelValuefor v-model,update:openfor new overlays.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/components/toggle/toggle.vue` around lines 79 - 81, The JSDoc for the toggleOnClick prop is stale: update the comment on toggleOnClick to replace "Change events will still be triggered" with wording that reflects the current event contract (mention that the component emits update:modelValue for v-model synchronization and, where applicable, update:open for overlays), so consumers know which events are emitted when toggleOnClick is false; keep the existing boolean description and `@values` tag unchanged and ensure the new text references the emitted events exactly as update:modelValue and update:open.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dialtone-vue/components/toggle/toggle.vue`:
- Around line 133-141: The emits array for DtToggle removed the 'change' event
but the commit lacks a BREAKING CHANGE footer—add a "BREAKING CHANGE: removed
'change' event from DtToggle; use update:modelValue instead" footer to the
commit/changelog; then update components/toggle/toggle.vue to remove or update
the stale comment that says "Change events will still be triggered" (reference
DtToggle and its emits array) and update
packages/dialtone-vue/components/toggle/toggle.stories.js to remove any
documentation/handlers referencing the 'change' event (replace with
update:modelValue handlers or examples) so docs and code are consistent with the
removal.
In `@packages/dialtone-vue/recipes/conversation_view/editor/editor.vue`:
- Around line 384-385: The public event contract was changed (legacy "input"
removed in favor of "update:modelValue" for the component that wires
`@update`:model-value="onUpdateModelValue" and `@selected`="onSelected"); add
explicit breaking-change metadata to the PR and commit by updating the commit
message/release notes to include a "BREAKING CHANGE:" footer that describes the
removal/rename of the "input" event and the new "update:modelValue" event
(mention the effected symbols onUpdateModelValue and onSelected and the
component editor.vue), and ensure the commit type reflects a breaking API change
(include the breaking change in the changelog/release body and classify the
commit appropriately so the release tooling treats it as a major/ breaking
release).
---
Outside diff comments:
In `@packages/dialtone-vue/components/toggle/toggle.vue`:
- Around line 79-81: The JSDoc for the toggleOnClick prop is stale: update the
comment on toggleOnClick to replace "Change events will still be triggered" with
wording that reflects the current event contract (mention that the component
emits update:modelValue for v-model synchronization and, where applicable,
update:open for overlays), so consumers know which events are emitted when
toggleOnClick is false; keep the existing boolean description and `@values` tag
unchanged and ensure the new text references the emitted events exactly as
update:modelValue and update:open.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 93e97495-2f98-4b09-8508-df1f47bbcba1
📒 Files selected for processing (3)
packages/dialtone-vue/components/checkbox_group/checkbox_group.test.jspackages/dialtone-vue/components/toggle/toggle.vuepackages/dialtone-vue/recipes/conversation_view/editor/editor.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dialtone-vue/components/checkbox_group/checkbox_group.test.js
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/dialtone-vue/common/mixins/input_group.js (1)
169-172:⚠️ Potential issue | 🟠 MajorPotential v-model contract mismatch + breaking-change communication gap.
At Line 171, the mixin emits
update:modelValue, but this mixin still definesvaluein props (Line 40). IfDtInputGroupdoes not explicitly bridge prop/event mapping, defaultv-modelbehavior can break for consumers. Also, removing/renaming public events requires explicitBREAKING CHANGErelease messaging in this repo.Use this to verify mapping and release-note compliance:
#!/bin/bash set -euo pipefail echo "== Verify prop/event pairing in mixin and component ==" for f in \ packages/dialtone-vue/common/mixins/input_group.js \ packages/dialtone-vue/components/input_group/input_group.vue do if [ -f "$f" ]; then echo "--- $f ---" rg -n -C3 "props\\s*:|model\\s*:|modelValue|value\\s*:|emits\\s*:|update:modelValue|input" "$f" fi done echo echo "== Verify current tests/stories expect the new contract ==" for f in \ packages/dialtone-vue/components/input_group/input_group.test.js \ packages/dialtone-vue/components/input_group/input_group.stories.js \ packages/dialtone-vue/components/input_group/input_group_default.story.vue \ apps/dialtone-documentation/docs/components/input-group.md do if [ -f "$f" ]; then echo "--- $f ---" rg -n -C2 "v-model|modelValue|update:modelValue|@input|onInput" "$f" fi done echo echo "== Verify commit footer contains BREAKING CHANGE ==" git log --format='%h %s%n%b%n---' -n 25 | rg -n "BREAKING CHANGE|DLT-3160|update:modelValue|input event"Expected result: either explicit
modelValue/mapping exists forDtInputGroup, or this PR is treated/versioned as breaking with a clearBREAKING CHANGEfooter.
As per coding guidelines, "This is a public npm library. Breaking changes without BREAKING CHANGE footer ship as patches and silently break consumers."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dialtone-vue/common/mixins/input_group.js` around lines 169 - 172, The mixin method setGroupValue currently emits 'update:modelValue' while the mixin/ component still declares the prop value (and DtInputGroup may rely on the older v-model/input contract); update the component and mixin to provide a consistent v-model contract by either (A) renaming the prop to modelValue and adding emits: ['update:modelValue'] (ensure DtInputGroup props include modelValue and the mapping), or (B) keep the prop named value but emit the legacy 'input' event alongside 'update:modelValue' from setGroupValue and add emits entries for both; also ensure tests/stories are updated to use modelValue or input accordingly and add a BREAKING CHANGE footer to the commit if you choose to change the public prop name (value -> modelValue) so consumers are informed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/dialtone-vue/common/mixins/input_group.js`:
- Around line 169-172: The mixin method setGroupValue currently emits
'update:modelValue' while the mixin/ component still declares the prop value
(and DtInputGroup may rely on the older v-model/input contract); update the
component and mixin to provide a consistent v-model contract by either (A)
renaming the prop to modelValue and adding emits: ['update:modelValue'] (ensure
DtInputGroup props include modelValue and the mapping), or (B) keep the prop
named value but emit the legacy 'input' event alongside 'update:modelValue' from
setGroupValue and add emits entries for both; also ensure tests/stories are
updated to use modelValue or input accordingly and add a BREAKING CHANGE footer
to the commit if you choose to change the public prop name (value -> modelValue)
so consumers are informed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e0a9053-3cfc-4245-aafd-34977f09f4eb
📒 Files selected for processing (3)
apps/dialtone-documentation/docs/components/banner.mdapps/dialtone-documentation/docs/components/toast.mdpackages/dialtone-vue/common/mixins/input_group.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dialtone-documentation/docs/components/toast.md
- apps/dialtone-documentation/docs/components/banner.md
Ignacio Ropolo (iropolo)
approved these changes
Apr 17, 2026
Contributor
Ignacio Ropolo (iropolo)
left a comment
Ignacio Ropolo (iropolo)
left a comment
There was a problem hiding this comment.
Some nit, looks good.
| await nativeInput.trigger('input'); | ||
|
|
||
| expect(wrapper.emitted().input).toBeUndefined(); | ||
| expect(wrapper.emitted()['update:modelValue']).toBeUndefined(); |
Contributor
There was a problem hiding this comment.
duplicated?
| expect(wrapper.emitted().input).toHaveLength(1); | ||
| expect(wrapper.emitted()['update:modelValue']).toHaveLength(1); | ||
| expect(wrapper.emitted().input[0][0]).toBe('か'); | ||
| expect(wrapper.emitted()['update:modelValue']).toHaveLength(1); |
Contributor
There was a problem hiding this comment.
duplicated too.
| await nativeTextarea.trigger('input'); | ||
|
|
||
| expect(wrapper.emitted().input).toBeUndefined(); | ||
| expect(wrapper.emitted()['update:modelValue']).toBeUndefined(); |
Contributor
There was a problem hiding this comment.
seems like there is some code duplicated along this PR
| await mountedRadioGroup.find(`[value="${MOCK_SELECTED_VALUE}"]`).trigger('change'); | ||
|
|
||
| expect(wrapper.emitted('input')).toBeFalsy(); | ||
| expect(wrapper.emitted('update:modelValue')).toBeFalsy(); |
Contributor
There was a problem hiding this comment.
Should it be mountWrapper instead of wrapper?
Comment on lines
217
to
219
| * If you set this value, the default mouseover behavior will be disabled and you can control it as you need. | ||
| * Supports .sync modifier | ||
| * @values null, true, false |
Contributor
There was a problem hiding this comment.
nit, .sync modifier is a vue2 pattern, this should be removed.
Contributor
Author
|
Changes made |
Contributor
|
✔️ Deploy previews ready! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Obligatory GIF (super important!)
🛠️ Type Of Change
📖 Jira Ticket
DLT-3160
📖 Description
Standardizes v-model event handling across all input and overlay components in
dialtone-vue, removing Vue 2 legacy patterns and correcting type declarations.Legacy
inputevent removed from 7 components —DtInput,DtCheckbox,DtRadio,DtRadioGroup,DtSelectMenu,DtRichTextEditor, andDtInputGroupdecorator. These were all emitting bothinput(Vue 2) andupdate:modelValue(Vue 3); theinputevent is no longer needed and has been dropped.DtCheckboxGroupmigrated from the non-standardselectedValues/update:selectedValuesnamed model to the standardmodelValue/update:modelValue, consistent withDtRadioGroupand all other form components.openprop type corrected on 5 overlay components (DtPopover,DtTooltip,DtDropdown,DtCollapsible,DtImageViewer) fromtype: Booleantotype: [Boolean, null]. These props acceptnullas a sentinel for uncontrolled mode, soBooleanwas a misrepresentation. Also removed stale Vue 2 "Supports .sync modifier" references from JSDoc.Toggle JSDoc cleaned up — removed stale
@modelannotations and fixed a wrong@eventname on theupdate:modelValueemit entry.All tests, stories, and story template files updated to reflect the changes.
📄 Documentation Artifacts
update:modelValueonInput→onUpdate:modelValueacross all affected stories📝 Checklist
For all PRs:
For all Vue changes: