Skip to content

Conversation

@gimmyhehe
Copy link
Member

@gimmyhehe gimmyhehe commented Dec 10, 2025

PR

修复拖拽源数据不会更新的问题,优化Insert和remove方法

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling and resolution of newly inserted rows in cell event operations.
  • Performance & Optimization

    • Optimized data tracking mechanisms for insert and removal operations in the grid.
    • Enhanced internal data structure management for better efficiency.

✏️ Tip: You can customize this high-level summary in your review settings.

@github-actions github-actions bot added the bug Something isn't working label Dec 10, 2025
@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

This PR refactors the grid's temporary row tracking mechanism from array-based storage to a Map-based approach (insertMap), updates row resolution logic in cell events, simplifies insertion/removal operations by removing auxiliary data copies, and adds utility methods for proxy unwrapping.

Changes

Cohort / File(s) Summary
Grid state and editStore refactoring
packages/vue/src/grid/src/table/src/table.ts, packages/vue/src/grid/src/table/src/methods.ts
Replaces temporaryRows array with insertMap (Map structure) in editStore. Initializes insertMap in loadTableData. Updates isTemporaryRow and hasRowInsert to check insertMap instead of array lookups. Adds deepUnwrap and cloneMapAndUnwrap utility methods for proxy handling. Changes updateRawData to direct assignment.
Edit operation refactoring
packages/vue/src/grid/src/edit/src/methods.ts
Simplifies operArrs signature from 8 parameters to 3 (newRecords, row, tableFullData), removing auxiliary array copies. Replaces temporary row tracking with insertMap.set() in insertion flow. Removes updateCache calls and nowData/tableSourceData/rawData manipulation. Updates _insertAt to conditionally mark inserted records and use new operArrs signature. Refactors _remove to rely on insertMap and isTemporaryRow instead of insertList.
Cell event resolution
packages/vue/src/grid/src/composable/useCellEvent.ts
Introduces rowid variable from rowEl.dataset?.rowid for virtual row detection. Adds logic to fetch newly inserted rows from insertMap when rowType is 'normal', falling back to getRowNode if not found. Replaces direct dataset checks with rowid-based conditionals.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas requiring extra attention:

  • Function signature change: operArrs parameter reduction from 8 to 3 — verify all call sites are updated correctly and data integrity is maintained without auxiliary array copies
  • insertMap initialization and access patterns: Ensure consistent usage across grid lifecycle and proper rowid derivation for map lookups
  • Method behavior changes: isTemporaryRow and hasRowInsert are likely used throughout the codebase; confirm all dependent code aligns with new insertMap-based implementation
  • deepUnwrap utility correctness: Verify recursive proxy unwrapping handles all edge cases, especially for nested Maps and Sets
  • Cell event resolution: Validate that fallback logic (insertMap → getRowNode) correctly resolves newly inserted vs. existing rows

Poem

🐰 A map replaces the list's old way,
Inserted rows now have their day,
No more arrays, just keyed delight,
Grid events flow swift and right! 🎯

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main changes: fixing data update issues and optimizing insert/remove operations, which align with the substantial refactoring across multiple files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cgm/optimize-insert-remove

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a0c2c73 and 916fe12.

📒 Files selected for processing (4)
  • packages/vue/src/grid/src/composable/useCellEvent.ts (2 hunks)
  • packages/vue/src/grid/src/edit/src/methods.ts (6 hunks)
  • packages/vue/src/grid/src/table/src/methods.ts (4 hunks)
  • packages/vue/src/grid/src/table/src/table.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/vue/src/grid/src/edit/src/methods.ts (2)
packages/renderless/src/grid/utils/common.ts (1)
  • getRowid (39-42)
packages/vue-common/src/index.ts (1)
  • hooks (477-477)
⏰ 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 (9)
packages/vue/src/grid/src/table/src/table.ts (1)

349-349: LGTM - Map-based tracking for inserted rows.

Using a Map for inserted rows provides O(1) lookup by rowid, which is more efficient than scanning insertList arrays. The initialization is correctly placed alongside the existing insertList.

packages/vue/src/grid/src/composable/useCellEvent.ts (1)

30-42: LGTM - Proper resolution for newly inserted rows.

The logic correctly handles the case where newly inserted rows haven't been cached in fullDataRowIdData yet. By checking insertMap first, the cell event handler can properly resolve the row object for recently inserted rows.

packages/vue/src/grid/src/table/src/methods.ts (4)

297-297: LGTM - insertMap cleared on data load.

Properly resets insertMap to a new empty Map when loading table data, preventing stale inserted row references.


329-331: Verify the intent of direct assignment vs spread copy.

The change from this.rawData = [...datas] to this.rawData = datas means the table now holds a reference to the original array rather than a shallow copy. This is likely intentional to fix the "data not update" issue mentioned in the PR (drag source data updates), but it means external mutations to the original array will affect the table.


618-622: LGTM - Efficient temporary row check using Map.

Using insertMap.has(rowid) provides O(1) lookup compared to array scanning, improving performance for tables with many rows.


662-664: LGTM - Simplified delegation.

hasRowInsert now correctly delegates to isTemporaryRow, reducing code duplication and ensuring consistent behavior.

packages/vue/src/grid/src/edit/src/methods.ts (3)

42-63: LGTM - Simplified operArrs with Map-based tracking.

The function signature is appropriately simplified, and the Map-based tracking (insertMap.set) provides efficient lookup for inserted rows. The logic correctly handles all three insertion cases: append (row === -1), insert at position (row && row !== -1), and prepend (!row).


125-132: LGTM - Improved record creation with reactivity.

The approach of:

  1. Conditionally marking records with the inserted field for async column support
  2. Wrapping records with hooks.reactive() for Vue 3 reactivity
  3. Using defineField to ensure required fields are present

This ensures proper reactivity and field initialization for newly inserted rows.


186-212: LGTM - Consistent cleanup of insertList and insertMap.

The removal logic correctly:

  1. Uses isTemporaryRow (line 188) for consistency with the new Map-based approach
  2. Synchronizes removal from both insertList and insertMap (lines 208-212)

This ensures the two data structures remain in sync.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@zzcr zzcr merged commit 9bd5cba into dev Dec 10, 2025
10 of 11 checks passed
@zzcr zzcr deleted the cgm/optimize-insert-remove branch December 10, 2025 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants