Skip to content

Conversation

dhruvisompura
Copy link
Contributor

@dhruvisompura dhruvisompura commented Aug 27, 2025

This PR updates the summary panel to utilize the changes made to the layout manager that add support for pinning.

This change should resolve the rendering mismatch, and scrolling bugs we were seeing with searching and sorting in the summary panel.

Release Notes

New Features

  • N/A

Bug Fixes

  • N/A

QA Notes

@:data-explorer @:web @:win

Copy link

github-actions bot commented Aug 27, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:data-explorer @:web @:win

readme  valid tags

@softwarenerd softwarenerd force-pushed the softwarenerd/data-explorer-updates branch 2 times, most recently from a7f7f6e to 0877cf6 Compare September 2, 2025 18:50
Base automatically changed from softwarenerd/data-explorer-updates to main September 5, 2025 00:12
@dhruvisompura dhruvisompura force-pushed the feature/summary-panel-updates branch from d5d8040 to 55b88a3 Compare September 8, 2025 17:56
@dhruvisompura dhruvisompura marked this pull request as ready for review September 8, 2025 17:56
@dhruvisompura dhruvisompura force-pushed the feature/summary-panel-updates branch from 55b88a3 to 4271159 Compare September 8, 2025 18:03
@dhruvisompura dhruvisompura force-pushed the feature/summary-panel-updates branch from d05c5e2 to b4996ef Compare September 9, 2025 16:24
await dataExplorer.summaryPanel.search('snickerdoodle');
await dataExplorer.summaryPanel.expectColumnCountToBe(0);
// await dataExplorer.summaryPanel.expectEmptyState(); // <--- no empty state created in UI yet
//await dataExplorer.summaryPanel.expectEmptyState(); // <--- no empty state created in UI yet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@midleman I enabled the e2e tests we were skipping since they should be passing now. I left this empty state check commented out since we don't have any special UI for that. I don't think we plan to add any UI for this state at this point so we may just want to remove this check entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, you are welcome to remove or when I come back for round 2 of updating/adding to these tests I can do it then. 🎉

Comment on lines -694 to +690
await this.clearColumnSortingButton.click();
await this.sortBy('Original');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@midleman We haven't added any UI affordances for clearing the summary panel sort. The only way to do that right now is by setting the sort value back to the original sort value. I have updated this helper to do that here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

* @param endIndex The end index.
* @returns An array with the specified index range.
*/
export const arrayFromIndexRange = (startIndex: number, endIndex: number) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The columnSchemaCache file is the only one that uses this function. It looks like @softwarenerd copied this function into that file. I'm removing this from the util file since its not used by any of the other data explorer cache files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of the changes in this file involve undoing the index mapping changes I made here since those now live in the layout manager. this file should be almost back to parity with how it looked prior to any of my changes. The main changes are in the update function now where we handle the search/sort params

* Constants.
*/
const TRIM_CACHE_TIMEOUT = 3000;
const OVERSCAN_FACTOR = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed because we determine the overscan total of rows/columns in the layer above the cache (the summary panel instance layer).

* behind the USE_DATA_EXPLORER_SUMMARY_PANEL_ENHANCEMENTS_KEY setting
* @param updateDescriptor The update descriptor containing the new search and sort parameters
*/
async update2(updateDescriptor: UpdateDescriptor): Promise<void> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer need a separate update function since the logic for handling search/sort is quite simple with all of the mapping logic moving into the layout manager.

* Updates the layout entries.
* @param state The backend state, if known; otherwise, undefined.
*/
const updateLayoutEntries = async (state?: BackendState) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function has been moved lower down in the file and handles the search/sort.

@dhruvisompura dhruvisompura force-pushed the feature/summary-panel-updates branch from 1668da9 to bbaa984 Compare September 9, 2025 16:54
midleman
midleman previously approved these changes Sep 9, 2025
Copy link
Contributor

@midleman midleman left a comment

Choose a reason for hiding this comment

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

The test updates look good, I will expand these tests today/tomorrow!

Copy link
Contributor

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

This is looking so good!

I have a question about some behavior I am seeing here:

blank-summary-panel.mov

If you first search and then try to resize some part of the waffle, I see the summary panel blank out.

I am happy to have this fixed in a followup PR.

@dhruvisompura
Copy link
Contributor Author

dhruvisompura commented Sep 9, 2025

This is looking so good!

I have a question about some behavior I am seeing here:

blank-summary-panel.mov
If you first search and then try to resize some part of the waffle, I see the summary panel blank out.

I am happy to have this fixed in a followup PR.

I noticed this issue as well and am looking into it! I'd prefer resolving this in a follow up if we're okay with that (most likely in the draft PR I have up here: #9220).

isabelizimm
isabelizimm previously approved these changes Sep 9, 2025
Copy link
Contributor

@isabelizimm isabelizimm left a comment

Choose a reason for hiding this comment

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

this lgtm with the follow up work outlined by others here 💯

@dhruvisompura
Copy link
Contributor Author

@midleman It looks like one of the failing e2e tests is caused by the bug that me and @juliasilge noticed. I'm going to try and push a fix for that in this branch before merging to avoid so we don't have to deal with a failing e2e test.

@dhruvisompura dhruvisompura dismissed stale reviews from isabelizimm and midleman via ce3feef September 9, 2025 23:57
@dhruvisompura dhruvisompura merged commit 0d59fe5 into main Sep 10, 2025
11 checks passed
@dhruvisompura dhruvisompura deleted the feature/summary-panel-updates branch September 10, 2025 22:24
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 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.

4 participants