[WIP]fix(dialog-select): dialog-select set-selection render checkbox error #3703
Conversation
WalkthroughAdds new grid TypeScript interfaces and a Partial type, tightens a DialogSelect prop validator to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as Test Runner
participant Dialog as DialogSelect Component
participant Grid as Grid Renderer
Tester->>Dialog: open dialog, trigger selection actions
Dialog->>Grid: toggle/select rows (index 1, then others)
Grid-->>Dialog: emit updated selection state
Dialog-->>Tester: render rows with `row__selected` classes
rect rgba(200,230,255,0.28)
note right of Dialog: Test expectation changed to include index 0 and 1 (i <= 1)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/sites/demos/pc/app/dialog-select/set-selection.spec.ts (2)
11-19: Stabilize assertions with auto-waiting and explicit selected count.Use locator-based expect with built-in waiting and assert exactly two selected rows; this reduces flakiness and improves clarity.
- const trs = await page.locator('.tiny-grid table tbody tr').all() - for (let i = 0; i < trs.length; i++) { - const classes = await trs[i].getAttribute('class') - if (i <= 1) { - expect(classes?.includes('row__selected')).toBeTruthy() - } else { - expect(classes?.includes('row__selected')).toBeFalsy() - } - } + const rows = page.locator('.tiny-grid table tbody tr') + await expect(rows.nth(1)).toBeVisible() // ensure at least 2 rows rendered + const selectedRows = page.locator('.tiny-grid table tbody tr.row__selected') + await expect(selectedRows).toHaveCount(2) + await expect(rows.nth(0)).toHaveClass(/row__selected/) + await expect(rows.nth(1)).toHaveClass(/row__selected/)
7-7: Make the Close button selector i18n-safe.Avoid hard-coding English-only label; match common locales.
- await page.getByRole('button', { name: 'Close' }).click() + await page.getByRole('button', { name: /Close|关闭/i }).click()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/sites/demos/pc/app/dialog-select/set-selection.spec.ts(1 hunks)
🔇 Additional comments (1)
examples/sites/demos/pc/app/dialog-select/set-selection.spec.ts (1)
14-18: LGTM: expecting two selected rows is correct.Broadening the condition to i <= 1 aligns with the scenario (first row preselected + toggled second row).
gausszhou
changed the title
gausszhou
force-pushed
the
zcg/e2e-set-selection
branch
from
c041392 to
c6991e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
examples/sites/demos/pc/app/dialog-select/set-selection.spec.ts (1)
14-18: Broadened expectation to two selected rows — confirm intent and harden the assertion loop.If the spec now expects rows 0 and 1 selected, add a guard to ensure at least two rows and switch to locator-based class checks to avoid flakiness from ElementHandle snapshots.
- const trs = await page.locator('.tiny-grid table tbody tr').all() - for (let i = 0; i < trs.length; i++) { - const classes = await trs[i].getAttribute('class') - if (i <= 1) { - expect(classes?.includes('row__selected')).toBeTruthy() - } else { - expect(classes?.includes('row__selected')).toBeFalsy() - } - } + const rows = page.locator('.tiny-grid table tbody tr') + const count = await rows.count() + expect(count).toBeGreaterThanOrEqual(2) + for (let i = 0; i < count; i++) { + const row = rows.nth(i) + if (i <= 1) { + await expect(row).toHaveClass(/row__selected/) + } else { + await expect(row).not.toHaveClass(/row__selected/) + } + }packages/vue/src/dialog-select/src/index.ts (1)
26-30: Centralize allowed values to avoid drift and improve type reuse.const $constants = { INPUT_HEIGHT: '32px', INPUT_MARGIN_BOTTOM: '8px' } +const POP_SELECTOR_VALUES = ['grid', 'tree'] as const + export default defineComponent({ @@ popseletor: { type: String, default: 'grid', - validator: (value: string) => ['grid', 'tree'].includes(value) + validator: (value: string) => (POP_SELECTOR_VALUES as readonly string[]).includes(value) },packages/renderless/types/grid.type.ts (1)
1-3: AvoidFunctiontype; provide a callable signature.At minimum, avoid
Function. If the shape isn’t final, use a generic callable.export interface IGridEvent { - selectAll: Function + // refine signature if known, e.g., (checked: boolean) => void + selectAll?: (...args: any[]) => void }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/sites/demos/pc/app/dialog-select/set-selection.spec.ts(1 hunks)packages/renderless/types/grid.type.ts(1 hunks)packages/vue/src/dialog-select/src/index.ts(1 hunks)
🔇 Additional comments (2)
packages/vue/src/dialog-select/src/index.ts (1)
29-29: Validator simplification toincludes— LGTM.Cleaner and returns a real boolean.
packages/renderless/types/grid.type.ts (1)
15-16: Make treeConfig/selectConfig optionalFile: packages/renderless/types/grid.type.ts (lines 15-16)
DialogSelect.gridOp defaults only provide columns/data; making these required will cause type friction across the codebase—mark them optional unless there’s a hard runtime requirement.
- treeConfig: any - selectConfig: any + treeConfig?: any + selectConfig?: any
gausszhou
force-pushed
the
zcg/e2e-set-selection
branch
from
c6991e6 to
d22e667
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/renderless/types/grid.type.ts (1)
6-7: Typo in column interface:filed→field(breaking).This will mislead consumers and break autocomplete. Rename now.
export interface IGridColumn { - filed: string + field: string title: string }Run to find any lingering usages:
#!/bin/bash rg -nP --type=ts -C2 '\bfiled\b'
🧹 Nitpick comments (3)
packages/renderless/types/grid.type.ts (3)
10-17: Tighten config typing; avoidany, add defaults, and mark optional where appropriate.Safer and more future-proof without breaking callers (defaults keep it permissive).
-export interface IGridConfig { - columns: IGridColumn[] - data: any[] - border: boolean - event: IGridEvent - treeConfig: any - selectConfig: any -} +export interface IGridConfig<TRow = unknown> { + columns: Array<IGridColumn<TRow>> + data: TRow[] + border?: boolean + event?: Partial<IGridEvent> + treeConfig?: unknown + selectConfig?: unknown +}And make
IGridColumngeneric to linkfieldto row keys (non-breaking with default):-export interface IGridColumn { - field: string - title: string -} +export interface IGridColumn<TRow = unknown> { + field: keyof TRow & string + title: string +}
1-19: Add minimal JSDoc on public types.Improves IDE hovers and guides external users.
Example:
/** Column definition for Grid */ export interface IGridColumn<TRow = unknown> { /** Property name in row data */ field: keyof TRow & string /** Display title */ title: string }
19-19: RenameIGriOp→IGridOptionsand keep a deprecated aliasTypo in the public API name; safe to rename — search shows only one occurrence at packages/renderless/types/grid.type.ts:19.
-export type IGriOp = Partial<IGridConfig> +export type IGridOptions<TRow = unknown> = Partial<IGridConfig<TRow>> +/** @deprecated Use IGridOptions */ +export type IGriOp<TRow = unknown> = IGridOptions<TRow>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/sites/demos/pc/app/dialog-select/set-selection.spec.ts(1 hunks)packages/renderless/types/grid.type.ts(1 hunks)packages/vue/src/dialog-select/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/sites/demos/pc/app/dialog-select/set-selection.spec.ts
- packages/vue/src/dialog-select/src/index.ts
| export interface IGridEvent { | ||
| selectAll: Function | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid Function type; use a concrete handler signature (or unknown args) and make it optional.
Improves type-safety and DX.
export interface IGridEvent {
- selectAll: Function
+ // TODO: refine args once confirmed (e.g., (checked: boolean) => void)
+ selectAll?: (...args: unknown[]) => void
}If selectAll is (checked: boolean) => void, confirm and we’ll lock that in.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "--- rg version ---"
rg --version 2>/dev/null || true
echo
echo "--- Search for IGridEvent ---"
rg -n --hidden --no-ignore -S '\bIGridEvent\b' -C3 || true
echo
echo "--- Show packages/renderless/types/grid.type.ts (if present) ---"
if [ -f packages/renderless/types/grid.type.ts ]; then
sed -n '1,240p' packages/renderless/types/grid.type.ts
else
echo "packages/renderless/types/grid.type.ts not found"
fi
echo
echo "--- Search for 'selectAll' occurrences ---"
rg -n --hidden --no-ignore -S '\bselectAll\b' -C3 || true
echo
echo "--- Search for '.selectAll(' invocations ---"
rg -n --hidden --no-ignore -S '\.selectAll\s*\(' -C2 || true
echo
echo "--- Search for property assignments like 'selectAll: (...) =>', 'selectAll: function' ---"
rg -n --hidden --no-ignore -S 'selectAll\s*:\s*(async\s*)?(\(?\s*[A-Za-z0-9_,\s]*\)?\s*=>|function)' -C3 || true
echo
echo "--- Search for emit/on patterns using literal 'selectAll' ---"
rg -n --hidden --no-ignore -S "emit('selectAll" -C2 || true
rg -n --hidden --no-ignore -S 'emit(\"selectAll' -C2 || true
rg -n --hidden --no-ignore -S "on('selectAll" -C2 || true
rg -n --hidden --no-ignore -S 'on(\"selectAll' -C2 || true
rg -n --hidden --no-ignore -S 'onSelectAll' -C2 || trueLength of output: 12394
Use explicit optional handler type for IGridEvent.selectAll
selectAll is invoked as selectAll({ selection, checked, $table }, event) (see packages/renderless/src/dialog-select/index.ts) — replace Function with a concrete optional signature.
export interface IGridEvent {
- selectAll: Function
+ // invoked as: selectAll({ selection, checked, $table }, event)
+ selectAll?: (payload: { selection: any[]; checked: boolean; $table: any }, event?: any) => void
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface IGridEvent { | |
| selectAll: Function | |
| } | |
| export interface IGridEvent { | |
| // invoked as: selectAll({ selection, checked, $table }, event) | |
| selectAll?: (payload: { selection: any[]; checked: boolean; $table: any }, event?: any) => void | |
| } |
🤖 Prompt for AI Agents
In packages/renderless/types/grid.type.ts around lines 1-3, IGridEvent.selectAll
is currently typed as Function; change it to an optional, explicit handler
signature that matches its usage selectAll({ selection, checked, $table },
event). Replace Function with something like selectAll?: (payload: { selection:
any[]; checked: boolean; $table: unknown }, event?: Event) => void (adjust
selection/$table types to more specific types from the codebase if available).
| export interface IGridConfig { | ||
| columns: IGridColumn[] | ||
| data: any[] | ||
| border: boolean | ||
| event: IGridEvent | ||
| treeConfig: any |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm property name event vs events.
Ensure this matches component API to avoid mismatch with consumers.
🏁 Script executed:
#!/bin/bash
# Look for grid config objects to confirm key naming conventions
rg -nP --type=ts -C2 '\bevents?\s*:' Length of output: 96763
Align property name: change IGridConfig.event → events
IGridConfig declares event: IGridEvent (packages/renderless/types/grid.type.ts:14) but the Vue component props use events (packages/vue/src/grid/src/grid/grid.ts:124). Make the property name consistent (rename to events or add an alias in types/component).
🤖 Prompt for AI Agents
In packages/renderless/types/grid.type.ts around lines 10 to 15, the IGridConfig
interface currently declares `event: IGridEvent` but the Vue component expects
`events`; rename the property to `events: IGridEvent` (or add an `events` alias
typed as IGridEvent and mark `event` deprecated) and update any imports/usages
to use the new property name so the type and component props are consistent.
gausszhou
force-pushed
the
zcg/e2e-set-selection
branch
from
d22e667 to
6ffd004
Compare
gausszhou
changed the title
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit