Skip to content

fix: DH-21658: Fix grid rendering hidden columns#2626

Merged
dgodinez-dh merged 5 commits intodeephaven:mainfrom
dgodinez-dh:dag_HiddenColumns2
Mar 5, 2026
Merged

fix: DH-21658: Fix grid rendering hidden columns#2626
dgodinez-dh merged 5 commits intodeephaven:mainfrom
dgodinez-dh:dag_HiddenColumns2

Conversation

@dgodinez-dh
Copy link
Copy Markdown
Contributor

@dgodinez-dh dgodinez-dh commented Feb 24, 2026

Two performance optimizations:

  • grid renderer should not iterate over hidden columns
  • metrics should not recalculate the width of a hidden column

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 24, 2026

Codecov Report

❌ Patch coverage is 59.09091% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.77%. Comparing base (c46fbaf) to head (7f20758).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
packages/grid/src/GridMetricCalculator.ts 40.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2626      +/-   ##
==========================================
- Coverage   49.78%   49.77%   -0.02%     
==========================================
  Files         774      774              
  Lines       43767    43781      +14     
  Branches    11256    11079     -177     
==========================================
+ Hits        21789    21791       +2     
- Misses      21960    21972      +12     
  Partials       18       18              
Flag Coverage Δ
unit 49.77% <59.09%> (-0.02%) ⬇️

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.

Comment thread packages/grid/src/GridMetrics.ts Outdated
Comment on lines +136 to +139
// Array of visible, non-hidden columns, by grid index
// Hidden columns may be included in visibleColumns and visibleColumnWidths with a width of 0 because users can see them and adjust their width
// But we do not want the grid renderer to iterate over them, so we provide this separate array
visibleNonHiddenColumns: readonly VisibleIndex[];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there possibly a better way to handle this? It just feels gross to have visible and nonHidden in the same thing because they're pretty much the same thing. I know visible really means "in the viewport" though

Also just creates a blurry line of which metric should I use

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions. It seems to me that we need both metrics. I did a version of this where a filtered visibileColumns in the renderer, but it seemed like a better optimization to calculate it ahead of time (especially since it is easy to add to the existing lopp).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I renamed this metric and changed the comment for more clarity.

Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few small comments to update

Comment thread packages/grid/src/GridMetricCalculator.ts Outdated
Comment thread packages/grid/src/GridMetrics.ts Outdated
@dgodinez-dh dgodinez-dh requested a review from mattrunyon March 5, 2026 14:10
@dgodinez-dh dgodinez-dh merged commit e54c0ac into deephaven:main Mar 5, 2026
11 checks passed
@dgodinez-dh dgodinez-dh deleted the dag_HiddenColumns2 branch March 17, 2026 18:45
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