Refactor row calculation logic in expandable.ts

[annilq]

bug:if the container set with a flex css property,below rowHeights will get a ['none'] value.

    const rowHeights = containerStyles
      .getPropertyValue('grid-template-rows')
      .split(' ');

Refactor : Optimize row calculation logic for extensible forms Replace the original cumulative height calculation with a mapping scheme based on element positions, improving the accuracy of row number assignment. Added boundary checks for containers to avoid potential errors.

Description

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update
• Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Checklist

ℹ️ Check all checkboxes - this will indicate that you have done everything in accordance with the rules in CONTRIBUTING.

• If you introduce new functionality, document it. You can run documentation with pnpm run docs:dev command.
• Run the tests with pnpm test.
• Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.
• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

Summary by CodeRabbit

• Refactor
  • Optimized row positioning logic in expandable form components for improved calculation accuracy and reliability.

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

[changeset-bot]

⚠️ No Changeset found

Latest commit: 866914e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

📝 Walkthrough

Walkthrough

The expandable form component's row calculation logic has been refactored from a grid-based template approach to a position-based assignment system using itemTop coordinates and a rowPositions map. The update includes bounding rect validation, improved collapsed row handling, and sequential row number assignment based on proximity matching.

Changes

Cohort / File(s)	Summary
Row calculation refactor packages/@core/ui-kit/form-ui/src/form-render/expandable.ts	Replaced grid-template-rows with position-based row assignment using itemTop and rowPositions map; added containerRect validation; introduced proximity matching (within 5px) for row determination; consolidated rowMapping updates for collapsed rows; restructured isCalculated timing

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• #6068 — Adds visibility-based recomputation trigger to the row-assignment mapping logic modified in this PR.

Suggested labels

bug

Suggested reviewers

• anncwb
• vince292007
• mynetfan
• jinmao88

Poem

🐰 Rows once lived in grids so neat,
Now they find their spots by heat—
Positions close shall share a place,
With bounding checks to set the pace! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The description identifies the bug, explains the refactoring approach, but most required checklist items remain unchecked and no type of change is selected.	Select the appropriate type of change (likely Bug fix), and check off checklist items to confirm testing, code review, and compliance with contribution guidelines.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main refactoring change in the pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

packages/@core/ui-kit/form-ui/src/form-render/expandable.ts (1)

91-93: Minor: Linear search creates array on each iteration.

[...rowPositions.entries()].find() allocates a new array for every form item. For typical form sizes this is negligible, but if forms grow large, consider using a helper function with early termination:

♻️ Optional optimization

+    // Helper to find existing row by position
+    const findRowByPosition = (top: number): number | undefined => {
+      for (const [position, row] of rowPositions) {
+        if (Math.abs(top - position) < 5) return row;
+      }
+      return undefined;
+    };
+
     formItems.forEach((el) => {
       const itemRect = el.getBoundingClientRect();
       const itemTop = itemRect.top - containerRect.top;

       let rowNumber = 1;
-      const existingRow = [...rowPositions.entries()].find(
-        ([position]) => Math.abs(itemTop - position) < 5,
-      );
+      const existingRow = findRowByPosition(itemTop);

       if (existingRow) {
-        rowNumber = existingRow[1];
+        rowNumber = existingRow;
       } else {

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 686c3f9 and 866914e.

📒 Files selected for processing (1)

• packages/@core/ui-kit/form-ui/src/form-render/expandable.ts

⏰ 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). (3)

• GitHub Check: Semantic Pull Request
• GitHub Check: post-update (ubuntu-latest)
• GitHub Check: post-update (windows-latest)

🔇 Additional comments (3)

packages/@core/ui-kit/form-ui/src/form-render/expandable.ts (3)

71-73: LGTM on the defensive check.

Good addition for robustness, even though getBoundingClientRect() typically returns a valid DOMRect for existing elements. This aligns with the PR goal of handling unexpected container styles.

---

91-101: Row assignment assumes DOM order matches visual (top-to-bottom) order.

The current logic assigns row numbers based on the order positions are first encountered (rowPositions.size + 1), not their actual vertical positions. This works correctly for typical grid/flex layouts where DOM order matches visual order, but could produce incorrect row numbers if CSS properties like order or flex-direction: column-reverse reorder elements visually.

For example, if the first DOM child is visually at the bottom, it would still be assigned row 1.

Consider if this edge case matters for your use case. If it does, sorting positions before assigning row numbers would fix it:

♻️ Optional fix for edge cases

-    // 单次遍历：收集位置、分配行号、统计数量
+    // First pass: collect unique positions
+    const positions = new Set<number>();
     formItems.forEach((el) => {
       const itemRect = el.getBoundingClientRect();
       const itemTop = itemRect.top - containerRect.top;
-
-      // 找到或分配行号
-      let rowNumber = 1;
-
-      // 检查是否已存在相近位置
-      const existingRow = [...rowPositions.entries()].find(
-        ([position]) => Math.abs(itemTop - position) < 5,
-      );
-
-      if (existingRow) {
-        rowNumber = existingRow[1];
-      } else {
-        // 如果是新位置，分配新行号
-        rowNumber = rowPositions.size + 1;
-        rowPositions.set(itemTop, rowNumber);
+      // Find existing nearby position or add new
+      const nearby = [...positions].find((p) => Math.abs(itemTop - p) < 5);
+      if (!nearby) {
+        positions.add(itemTop);
       }
+    });
 
-      // 统计每行元素数量（仅统计折叠行数内的）
-      if (rowNumber <= collapsedRows) {
-        rowMapping.value[rowNumber] = (rowMapping.value[rowNumber] ?? 0) + 1;
-      }
+    // Sort positions and create mapping
+    const sortedPositions = [...positions].sort((a, b) => a - b);
+    sortedPositions.forEach((pos, idx) => rowPositions.set(pos, idx + 1));
+
+    // Second pass: assign row numbers and count
+    formItems.forEach((el) => {
+      const itemRect = el.getBoundingClientRect();
+      const itemTop = itemRect.top - containerRect.top;
+      const [, rowNumber] = [...rowPositions.entries()].find(
+        ([position]) => Math.abs(itemTop - position) < 5,
+      ) ?? [0, 1];
+
+      if (rowNumber <= collapsedRows) {
+        rowMapping.value[rowNumber] = (rowMapping.value[rowNumber] ?? 0) + 1;
+      }
     });

---

75-110: Solid approach to replace grid-template-rows parsing.

The position-based row calculation is more robust than relying on grid-template-rows, which can return 'none' for flex containers as noted in the PR objectives. The 5px tolerance reasonably handles subpixel rendering differences.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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