Skip to content

feat: Add IrisGridCacheUtils for memoizing iris grid state#2416

Merged
mattrunyon merged 5 commits intodeephaven:mainfrom
mattrunyon:iris-grid-state-refactor
Apr 23, 2025
Merged

feat: Add IrisGridCacheUtils for memoizing iris grid state#2416
mattrunyon merged 5 commits intodeephaven:mainfrom
mattrunyon:iris-grid-state-refactor

Conversation

@mattrunyon
Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon commented Apr 21, 2025

Adds IrisGridCacheUtils which provides 3 methods to create memoized dehydrators: makeMemoizedGridStateDehydrator, makeMemoizedIrisGridStateDehydrator, and makeMemoizedCombinedGridStateDehydrator. The first 2 are used for IrisGridPanel which saves the state under separate keys for gridState and irisGridState. The 3rd is for newer uses like UITable which can just save the combined state and spread it to IrisGrid since IrisGrid accepts both GridState and IrisGridState props.

Added tests for dehydrateIrisGridState.

Improved the memoize-one type override to infer the types of the equality function based on the actual function args.

@mattrunyon mattrunyon requested a review from mofojed April 21, 2025 21:14
@mattrunyon mattrunyon self-assigned this Apr 21, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2025

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.

Project coverage is 46.92%. Comparing base (2533670) to head (05aa450).

Files with missing lines Patch % Lines
packages/iris-grid/src/IrisGridUtils.ts 33.33% 2 Missing ⚠️
...ashboard-core-plugins/src/panels/IrisGridPanel.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2416      +/-   ##
==========================================
+ Coverage   46.84%   46.92%   +0.08%     
==========================================
  Files         711      712       +1     
  Lines       39319    39340      +21     
  Branches     9826    10024     +198     
==========================================
+ Hits        18418    18462      +44     
+ Misses      20890    20824      -66     
- Partials       11       54      +43     
Flag Coverage Δ
unit 46.92% <92.68%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mattrunyon mattrunyon requested a review from Copilot April 21, 2025 22:49
Copy link
Copy Markdown
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 refactors the dehydration logic in IrisGridUtils by memoizing both grid state and Iris grid state conversion methods to improve performance and consistency. Key changes include updating type definitions and functions in IrisGridUtils.ts, revising corresponding tests in IrisGridUtils.test.ts, and simplifying state retrieval in IrisGridPanel.tsx.

Reviewed Changes

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

File Description
packages/iris-grid/src/IrisGridUtils.ts Added memoization for dehydrateGridState and dehydrateIrisGridState with updated type definitions and equality checks.
packages/iris-grid/src/IrisGridUtils.test.ts Updated and added tests to verify serialization and memoization behavior.
packages/dashboard-core-plugins/src/panels/IrisGridPanel.tsx Removed redundant memoization wrappers in favor of direct dehydration function calls.
Comments suppressed due to low confidence (2)

packages/iris-grid/src/IrisGridUtils.ts:1298

  • Ensure that the 'metrics' property is always defined in 'irisGridState' to avoid unintended cache invalidation within the equality function. If 'metrics' could be undefined, adjust the equality check or enforce its presence.
return (a[0] === b[0] && a[1].metrics != null && b[1].metrics != null && a[1].metrics.userColumnWidths === b[1].metrics.userColumnWidths && a[1].metrics.userRowHeights === b[1].metrics.userRowHeights && compareStateKeys.every(key => a[1][key] === b[1][key]));

packages/iris-grid/src/IrisGridUtils.test.ts:705

  • [nitpick] Consider adding tests that vary individual properties within 'irisGridState' to ensure the custom equality function in memoizeOne accurately detects changes. This will improve confidence in the memoization logic under edge cases.
test('dehydrateIrisGridState should be serializable and memoized', () => {

@mattrunyon mattrunyon requested a review from Copilot April 23, 2025 17:09
Copy link
Copy Markdown
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 refactors iris grid state dehydration by introducing memoization to improve performance and streamline state comparison in both grid and IrisGrid utilities. Key changes include:

  • Refactoring dehydrate/hydrate functions in IrisGridUtils with updated types and default fallback values.
  • Introducing new memoized cache utilities in IrisGridCacheUtils with corresponding tests.
  • Updating IrisGridPanel to leverage the new memoization functions for state dehydration.

Reviewed Changes

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

Show a summary per file
File Description
packages/iris-grid/src/index.ts Export added for IrisGridCacheUtils.
packages/iris-grid/src/IrisGridUtils.ts Updated types and refactored dehydration/hydration functions with improved defaults.
packages/iris-grid/src/IrisGridUtils.test.ts Updated tests to reflect new function signatures and naming.
packages/iris-grid/src/IrisGridCacheUtils.ts New cache utilities using memoization are provided.
packages/iris-grid/src/IrisGridCacheUtils.test.ts Tests for the memoization logic have been added/updated.
packages/dashboard-core-plugins/src/panels/IrisGridPanel.tsx Removed inline memoization in favor of the new cache utilities and updated state dehydration usage.
Comments suppressed due to low confidence (2)

packages/iris-grid/src/IrisGridUtils.test.ts:666

  • The test label 'dehydrateIrisGridState' does not match the function being invoked ('dehydrateGridState'); consider updating the label for consistency and clarity.
'dehydrateIrisGridState',

packages/iris-grid/src/IrisGridUtils.ts:1169

  • Confirm that the default fallback for metrics is intentional; using fallback values could mask cases where metrics are unexpectedly undefined.
metrics: { userColumnWidths, userRowHeights } = { userColumnWidths: EMPTY_MAP, userRowHeights: EMPTY_MAP },

isStuckToBottom: boolean;
isStuckToRight: boolean;
movedColumns: {
movedColumns: readonly {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yesssss readonly supreme.

@mattrunyon mattrunyon changed the title refactor: Memoize dehydrating iris grid states in IrisGridUtils feat: Add IrisGridCacheUtils for memoizing iris grid state Apr 23, 2025
@mattrunyon mattrunyon merged commit d6826ce into deephaven:main Apr 23, 2025
13 checks passed
@mattrunyon mattrunyon deleted the iris-grid-state-refactor branch April 23, 2025 22:15
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants