Skip to content

fix: row grouping algorithm#999

Merged
lukasmasuch merged 1 commit intoglideapps:mainfrom
Workfront:fix-grouping-issue
Jun 18, 2025
Merged

fix: row grouping algorithm#999
lukasmasuch merged 1 commit intoglideapps:mainfrom
Workfront:fix-grouping-issue

Conversation

@citizensas
Copy link
Contributor

  • Improved Row Index Mapping:
    Enhanced mapRowIndexToPath for edge cases and overflow.
  • Enhanced useRowGrouping Hook:
    Refined mapper function for type safety.

fix: typings for RowGroupingMapperResult
@lukasmasuch lukasmasuch requested a review from Copilot June 16, 2025 23:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes errors in the row grouping algorithm to better handle edge cases and improve type safety.

  • Refactors mapRowIndexToPath to fix an off‐by-one error in row mapping logic.
  • Updates the RowGroupingMapperResult type usage and associated tests to ensure accurate row index mapping.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/core/test/row-grouping-api.test.ts Adds comprehensive tests to verify row grouping behavior.
packages/core/src/data-editor/row-grouping.ts Adjusts index calculations in the row-mapping algorithm.
packages/core/src/data-editor/row-grouping-api.ts Refines type definitions for the row grouping mapper function.
Comments suppressed due to low confidence (2)

packages/core/src/data-editor/row-grouping.ts:203

  • Consider adding a brief inline comment explaining the '-1' adjustment in the spread 'path: [...group.path, toGo - 1]'. This would improve clarity regarding how the offset is calculated.
if (toGo <= group.rows)

packages/core/src/data-editor/row-grouping-api.ts:12

  • Avoid using 'as any' here by refining the type definitions of the mapper result; this would improve type safety and overall code maintainability.
return { ...r, originalIndex: [itemOrRow[0], r.originalIndex], } as any; // FIXME

Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@lukasmasuch lukasmasuch merged commit ee5c60e into glideapps:main Jun 18, 2025
@citizensas citizensas deleted the fix-grouping-issue branch July 20, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments