fix(grid): fix insert rowIndex error #3916
Conversation
WalkthroughCache handling moved from a backupInfos structure to a per-rowid Map and tableFullData is now the canonical source; updateCache() is invoked after record insertions/removals; scrollYLoad and tableFullData are initialized on rawDataVersion changes; a sticky-header CSS variable was adjusted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 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)
⏰ 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)
🔇 Additional comments (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: 0
🧹 Nitpick comments (1)
packages/vue/src/grid/src/table/src/methods.ts (1)
410-480: Consider performance optimization for bulk operations.The current implementation rebuilds the entire cache on every
updateCache()call. For typical single insert/remove operations this is fine, but if your grid supports bulk operations (inserting/removing many rows at once), you might want to consider:
- Batching: Only call
updateCache()once after bulk operations complete- Incremental updates: Update only affected row indexes instead of rebuilding the entire cache
The current implementation is correct and maintainable. This optimization would only be necessary if profiling shows performance issues with bulk operations.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/vue/src/grid/src/edit/src/methods.ts(2 hunks)packages/vue/src/grid/src/table/src/methods.ts(1 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). (2)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
- GitHub Check: verify-main-build
🔇 Additional comments (3)
packages/vue/src/grid/src/edit/src/methods.ts (2)
142-142: Correctly updates cache after inserting records.This ensures
fullDataRowIdDataandfullDataRowMapare rebuilt to include the newly inserted rows with correct indexes, fixing the rowIndex error described in the PR.
216-216: Correctly updates cache after removing records.This maintains cache consistency by recalculating row indexes for all remaining rows after removal, ensuring symmetry with the insert operation.
packages/vue/src/grid/src/table/src/methods.ts (1)
411-412: Critical fix: Cache now reflects current data state including insertions.Changing the data source from
rawDatatotableFullDatais essential for this fix:
rawData: Original data source (doesn't include inserted rows)tableFullData: Current table data (includes inserted rows)This ensures that when
updateCache()is called after insert operations, the cache includes the newly inserted rows with correct indexes.
gimmyhehe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vue/src/grid/src/table/src/methods.ts (1)
404-461: rowidCacheMap state loss due to requestIdleCallback overwriteThe updateCache method schedules a requestIdleCallback that creates a new local rowidCacheMap and overwrites
this.rowidCacheMapon line 461. This destroys any state changes made viasetOriginRow()between updateCache calls.For example, in
reloadRow()(lines 369-372):
setOriginRow(row, backupRow)modifies rowidCacheMap directlyupdateCache()schedules a callback- When the callback executes, it creates a fresh rowidCacheMap and overwrites the instance, losing the changes from setOriginRow
This pattern occurs in edit operations where
setOriginRow()andupdateCache()are called in sequence. The asynchronous requestIdleCallback callback can execute after subsequent synchronous edits, causing state loss.Consider deferring the rowidCacheMap assignment until the callback completes, or ensuring setOriginRow changes persist across updateCache invocations.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/theme-saas/src/grid/table.lesspackages/vue/src/grid/src/composable/useNormalData.tspackages/vue/src/grid/src/table/src/methods.tspackages/vue/src/grid/src/table/src/table.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-04T09:35:13.159Z
Learnt from: zzcr
Repo: opentiny/tiny-vue PR: 2481
File: packages/theme/src/time-range/vars.less:27-28
Timestamp: 2024-11-04T09:35:13.159Z
Learning: 在 `packages/theme/src/time-range/vars.less` 文件中，应使用 `var(--tv-TimeRange-header-height)` 作为 `--tv-TimeRange-header-line-height` 的值，以保持一致性。
Applied to files:
packages/theme-saas/src/grid/table.less
🧬 Code graph analysis (1)
packages/vue/src/grid/src/table/src/methods.ts (1)
packages/renderless/src/grid/utils/common.ts (1)
getRowid(39-42)
⏰ 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/theme-saas/src/grid/table.less (1)
1449-1449: PR scope mismatch: CSS styling change in rowIndex fix PR.This PR is titled "fix(grid): fix insert rowIndex error," yet the modification at line 1449 is a CSS variable styling change. CSS styling adjustments cannot address rowIndex logic issues.
The change itself appears safe—using
var(--tiny-color-fill-8, #f4f6fb)is consistent with lines 196, 201, and 231 elsewhere in the file. However, clarify whether:
- Other files in this PR contain the actual rowIndex logic fix
- This CSS change is incidental or directly related to the stated issue
packages/vue/src/grid/src/table/src/table.ts (2)
788-788: LGTM!Proper cleanup of the rowidCacheMap on unmount to prevent memory leaks.
811-811: LGTM!Introduction of rowidCacheMap provides a cleaner per-rowid caching mechanism, replacing the previous backupInfos approach. The Map is properly initialized and exposed via the setup return.
Also applies to: 847-848
packages/vue/src/grid/src/table/src/methods.ts (2)
291-291: LGTM!Removal of destructuring is appropriate since
tableFullDatais now set upstream in useNormalData.ts and accessed viathiswhen needed.
328-336: LGTM!The refactored origin row management using rowidCacheMap is cleaner and more direct. The null checks are appropriate and handle the case where getRowid returns an empty string.
packages/vue/src/grid/src/composable/useNormalData.ts (1)
67-75: The review comment flags a non-issue. Code is safe by design.The concern about accessing
scrollY.gtwithout checking ifscrollYexists is unfounded.scrollYis always defined in the defaultGlobalConfig.optimizationwithgt: 500. TheoptimizeOptscomputed property usesextend()to merge defaults with user props, ensuringscrollYandscrollY.gtalways exist. ThescrollY &&check on line 74, while redundant, does not cause harm. The suggested additional check (scrollY.gt &&) is unnecessary and based on incorrect assumptions about the code's safety.
PR
修复插入的数据rowIndex不正确问题
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
Bug Fixes
Improvements
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.