fix(grid): fix visible columns change wloud delete insert row #3897
Conversation
WalkthroughAdds a TinyGridToolbar to a demo grid; shifts internal restore/load sources to prefer rawData; derives editable columns from tableFullColumn and normalizes fields when that set changes; expands a demo test to toggle column visibility via the personalization panel. Changes
Sequence Diagram(s)sequenceDiagram
participant Demo as Demo (TinyGrid)
participant Toolbar as TinyGridToolbar
participant Table as Table instance
participant Composable as useNormalData
participant Editor as Edit methods
Demo->>Toolbar: render toolbar (localStorage settings)
Demo->>Table: initialize table (pass tableFullColumn)
Table->>Composable: useNormalData({ props, tableFullColumn })
Composable->>Composable: derive editorColumns from tableFullColumn
Composable->>Table: call table.defineField(row, copy, editorColumns, cache)
Note right of Table: defineField may add fields and sync origin rows when editorColumns changes
User->>Demo: trigger edit or revert
Demo->>Editor: invoke revert/edit
Editor->>Table: _revertData reads from rawData and reloads rows
Table->>Demo: updated rows/state rendered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/vue/src/grid/src/edit/src/methods.ts (1)
254-285: Avoid reloading with possibly-undefinedrawData
Ifthis.rawDatacan ever beundefined/non-array,return this.reloadData(rawData)(Line 284) can behave like “clear”. Normalize once and reuse._revertData(rows, field) { - let { rawData } = this + const rawData = Array.isArray(this.rawData) ? this.rawData : [] if (arguments.length && rows && !isArray(rows)) { rows = [rows] } if (!arguments.length) { - rows = rawData || [] + rows = rawData } @@ - return this.reloadData(rawData) + return this.reloadData(rawData) },packages/vue/src/grid/src/table/src/methods.ts (1)
753-765:getData()should not ignoretableFullDatawhenrawDatais unset/stale
const allRows = this.rawData || [](Line 754) can return[]even whentableFullDatais populated (e.g., vialoadTableData). Safer fallback:getData(rowIndex) { - const allRows = this.rawData || [] + const allRows = this.rawData || this.tableFullData || []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/sites/demos/pc/app/grid/data-source/undefined-field-defalut-value-composition-api.vue(2 hunks)examples/sites/demos/pc/app/grid/data-source/undefined-field-defalut-value.spec.js(2 hunks)examples/sites/demos/pc/app/grid/data-source/undefined-field-defalut-value.vue(2 hunks)packages/vue/src/grid/src/composable/useNormalData.ts(2 hunks)packages/vue/src/grid/src/edit/src/methods.ts(2 hunks)packages/vue/src/grid/src/table/src/methods.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (6)
packages/vue/src/grid/src/composable/useNormalData.ts (2)
3-21: Watcher split matches the bugfix intent (don’t reload on column changes)
Good change:rawDataVersionbumps only onrawDataupdates (Line 10-21), avoiding unintended reloads from column-visibility changes. Consider a small safety guard to avoid runtime errors if$tableisn’t ready:- const $table: any = hooks.getCurrentInstance()?.proxy + const $table: any = hooks.getCurrentInstance()?.proxy @@ - _data.forEach((row) => $table.defineField(row)) + $table && _data.forEach((row) => $table.defineField(row))
23-33: Ensure insertList rows are normalized without triggering a data reload
This watcher (Line 23-33) looks correct for the reported issue: it updates row shape forrawDataandeditStore.insertListwhile not bumpingrawDataVersion(so inserted rows shouldn’t be dropped).packages/vue/src/grid/src/table/src/methods.ts (1)
290-317:loadTableData()intentionally does not updaterawData—document its contract to prevent API misuse.The current design uses separate data paths:
loadTableData()updatestableFullDatafor display (e.g., filtered data in select/grid-select), whilerawDataremains the source of truth. External callers in select and grid-select do not callgetData()afterloadTableData(), so no active bug exists. However, the API contract is unclear. Future developers may incorrectly assumegetData()reflects changes fromloadTableData(). Consider:
- Document
loadTableData()as an internal method intended only for display updates (e.g., filtering) that do not affect the source data.- Alternatively, update
getData()to fall back totableFullDatawhenrawDatais unavailable or stale, ensuring consistency.examples/sites/demos/pc/app/grid/data-source/undefined-field-defalut-value.spec.js (1)
7-51: Selector scoping and test isolation concerns require HTML structure verificationThe test does use
page.locator('.tiny-grid-custom__setting-btn')without scoping todemo, which could cause cross-demo collisions if multiple grid instances exist on the page. However, the proposed fix of scoping todemo.locator('.tiny-grid-custom__setting-btn')assumes the settings button is a child of the demo container—this needs verification against the actual HTML structure.Regarding localStorage: Playwright does not clear localStorage by default on
page.goto(), so persisted column visibility settings could cause test flakiness across runs. However,localStorage.clear()is only necessary if the grid customization state is actually stored in localStorage.Before applying the suggested fixes, confirm:
- Whether
.tiny-grid-custom__setting-btnis nested inside the demo element or is a global component- Whether the grid custom settings are persisted via localStorage or session state
examples/sites/demos/pc/app/grid/data-source/undefined-field-defalut-value.vue (2)
28-28: LGTM!The import statement correctly includes TinyGridToolbar, which is necessary for the new toolbar functionality.
35-36: LGTM!Component registrations are correct. Both TinyButton (which was previously used in the template) and the newly added TinyGridToolbar are now properly registered.
| <template #toolbar> | ||
| <tiny-grid-toolbar id="undefined-field-defalut-value" :setting="{ storage: 'local' }"></tiny-grid-toolbar> | ||
| </template> |
There was a problem hiding this comment.
Typo in the toolbar ID attribute.
The ID contains "defalut" instead of "default". While this is consistent with the filename, it's still a typo that should be corrected.
Apply this diff to fix the typo:
<template #toolbar>
- <tiny-grid-toolbar id="undefined-field-defalut-value" :setting="{ storage: 'local' }"></tiny-grid-toolbar>
+ <tiny-grid-toolbar id="undefined-field-default-value" :setting="{ storage: 'local' }"></tiny-grid-toolbar>
</template>Note: You may also want to rename the file itself to maintain consistency.
📝 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.
| <template #toolbar> | |
| <tiny-grid-toolbar id="undefined-field-defalut-value" :setting="{ storage: 'local' }"></tiny-grid-toolbar> | |
| </template> | |
| <template #toolbar> | |
| <tiny-grid-toolbar id="undefined-field-default-value" :setting="{ storage: 'local' }"></tiny-grid-toolbar> | |
| </template> |
🤖 Prompt for AI Agents
In
examples/sites/demos/pc/app/grid/data-source/undefined-field-defalut-value.vue
around lines 5 to 7, the toolbar id contains a typo "defalut" instead of
"default"; update the id attribute value to "undefined-field-default-value" to
correct the spelling, and optionally rename the file to
examples/sites/demos/pc/app/grid/data-source/undefined-field-default-value.vue
(and update any imports/references) to keep filename consistent with the
corrected id.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/vue/src/grid/src/composable/useNormalData.ts(1 hunks)packages/vue/src/grid/src/edit/src/methods.ts(2 hunks)packages/vue/src/grid/src/table/src/table.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vue/src/grid/src/edit/src/methods.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/vue/src/grid/src/table/src/table.ts (1)
packages/vue/src/grid/src/composable/useNormalData.ts (1)
useNormalData(3-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (6)
packages/vue/src/grid/src/composable/useNormalData.ts (5)
3-3: LGTM! API signature updated correctly.The refactoring from
visibleColumntotableFullColumnaligns with the PR objective to fix the issue where changingvisibleColumnwould delete newly inserted rows. By tracking the full column set instead of just visible columns, the data normalization can react properly to column visibility changes.
4-4: LGTM! Explicit typing improves clarity.Adding the explicit type annotation for
$tableimproves code readability, even though it's typed asany.
9-11: LGTM! New state for tracking editable columns.The introduction of
editorColumnsandeditorColumnKeyenables the new logic to track which columns are editable and react to changes in the editable column set.
47-59: LGTM! Raw data normalization logic is sound.The watcher correctly normalizes fields for
rawDataonly (notinsertList), as indicated by the comment that temporary insert data will be cleaned up later. The version increment triggers downstream cache updates and data handling via the existingrawDataVersionwatcher.
34-45: No conflict between watchers—the design is intentional.The
editorColumnswatcher processes bothinsertListandrawDatabecause column structure changes require normalizing all data. The subsequentrawDatawatcher explicitly excludesinsertList, as documented in its inline comment: "不处理临时插入数据，因为后续会被清理掉" (doesn't process insertList because it will be cleaned up later). TheinsertListis cleared when table data is loaded vialoadTableData(), confirming it's temporary and the design is sound.packages/vue/src/grid/src/table/src/table.ts (1)
711-711: LGTM! Integration point updated correctly.The change from
visibleColumntotableFullColumncorrectly aligns with the refactoreduseNormalDataAPI. This enables the data normalization logic to track all columns (not just visible ones), which should fix the reported issue where changingvisibleColumnwould delete newly inserted rows.
| hooks.watch(tableFullColumn, (_tableFullColumn) => { | ||
| let columns = [] | ||
| let columnKeys = [] | ||
|
|
||
| for (let i = 0; i < _tableFullColumn.length; i++) { | ||
| const column = _tableFullColumn[i] | ||
|
|
||
| if (column.property && column.editor) { | ||
| columns.push(column) | ||
| columnKeys.push(column.propety) | ||
| } | ||
| } | ||
|
|
||
| const keyString = columnKeys.sort().join() | ||
|
|
||
| if (editorColumnKey.value !== keyString) { | ||
| editorColumnKey.value = keyString | ||
| editorColumns.value = columns | ||
| } | ||
| }) |
There was a problem hiding this comment.
Critical typo: column.propety should be column.property.
Line 22 contains a typo that will cause the columnKeys array to collect undefined values, breaking the key comparison logic and potentially causing incorrect behavior when columns change.
Apply this diff to fix the typo:
if (column.property && column.editor) {
columns.push(column)
- columnKeys.push(column.propety)
+ columnKeys.push(column.property)
}📝 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.
| hooks.watch(tableFullColumn, (_tableFullColumn) => { | |
| let columns = [] | |
| let columnKeys = [] | |
| for (let i = 0; i < _tableFullColumn.length; i++) { | |
| const column = _tableFullColumn[i] | |
| if (column.property && column.editor) { | |
| columns.push(column) | |
| columnKeys.push(column.propety) | |
| } | |
| } | |
| const keyString = columnKeys.sort().join() | |
| if (editorColumnKey.value !== keyString) { | |
| editorColumnKey.value = keyString | |
| editorColumns.value = columns | |
| } | |
| }) | |
| hooks.watch(tableFullColumn, (_tableFullColumn) => { | |
| let columns = [] | |
| let columnKeys = [] | |
| for (let i = 0; i < _tableFullColumn.length; i++) { | |
| const column = _tableFullColumn[i] | |
| if (column.property && column.editor) { | |
| columns.push(column) | |
| columnKeys.push(column.property) | |
| } | |
| } | |
| const keyString = columnKeys.sort().join() | |
| if (editorColumnKey.value !== keyString) { | |
| editorColumnKey.value = keyString | |
| editorColumns.value = columns | |
| } | |
| }) |
🤖 Prompt for AI Agents
In packages/vue/src/grid/src/composable/useNormalData.ts around lines 13 to 32
there is a typo: column.propety is used when pushing keys which yields
undefined; change that to column.property so columnKeys collects the actual
property values (ensure the push uses column.property and nothing else needs
changing).
gimmyhehe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/sites/demos/pc/app/grid/custom/column-simple.spec.js (1)
7-7: Prefer role-based selectors for better test resilience.The shift from
getByRole('listitem')to CSS class selector.tiny-option-labelmakes the test more brittle. Role-based selectors are Playwright's recommended approach—they're semantic, accessibility-focused, and less likely to break when implementation details change.Consider reverting to role-based selectors:
- await page.locator('.tiny-option-label').filter({ hasText: '员工数' }).click() + await page.getByRole('listitem', { name: '员工数' }).click()- await page.locator('.tiny-option-label').filter({ hasText: '名称' }).click() + await page.getByRole('listitem', { name: '名称' }).click()If the UI lacks proper ARIA roles, consider fixing the component markup instead of downgrading the test selectors.
Also applies to: 10-10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/sites/demos/pc/app/grid/custom/column-simple.spec.js(1 hunks)examples/sites/demos/pc/app/grid/editor/custom-editor-select.spec.js(1 hunks)
🔇 Additional comments (1)
examples/sites/demos/pc/app/grid/editor/custom-editor-select.spec.js (1)
6-10: No changes needed. Line 9 correctly usespage.locator('.tiny-option-label')because the select component's dropdown options are rendered in the document body by default (viapopperAppendToBody: true), not within the demo container. Thedemo.locator()scoping on preceding lines applies only to elements within the component; usingpage.locator()for portal elements rendered outside is the intended pattern.Likely an incorrect or invalid review comment.
PR
修复visibleColumn变化时，会删除掉新插入数据问题
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
New Features
Bug Fixes
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.