maester365
fix #949: now a single dialog opens when you click a result.
The ResultInfoDialog was opening multiple times because each row in the TestResultsTable was designed to have several clickable elements (ID, Title, and an Info button) that could each trigger a dialog for the same test result item.
Initially, the logic to determine if a dialog should be open was based on a shared identifier for the row's item (like item.Id or item.Index). When you clicked one element, the condition to open the dialog became true for all ResultInfoDialog instances associated with that same item's Index, causing all of them to appear.
The fix involved changing the activeDialog state in TestResultsTable.jsx to store a more specific identifier (a unique dialogId string like dialog-${item.Index}-id, dialog-${item.Index}-title, or dialog-${item.Index}-button). Each ResultInfoDialog instance was then given its own unique dialogId. The isOpen prop for each dialog was then determined by checking if the global activeDialog state matched its specific dialogId. This ensured that only the one specific dialog instance that was clicked (or navigated to) would open.
Only one dialog opens now, however there is still quite the performance hit when the report contains several hundred tests. Do you also experience a delay when opening and closing dialogs, if the report contains hundreds of tests? I also see msedge.exe "spike" on a single core.
Before refactoring it takes approximately 80-100ms, and sometimes down to 40ms, to open a dialog.:
After refactoring:
This should be taken into considerations if you can reproduce it.
Only one dialog opens now, however there is still quite the performance hit when the report contains several hundred tests. Do you also experience a delay when opening and closing dialogs, if the report contains hundreds of tests? I also see msedge.exe "spike" on a single core.
Before refactoring it takes approximately 80-100ms, and sometimes down to 40ms, to open a dialog.
This should be taken into considerations if you can reproduce it.
@tdcthosc O Wow. That's a huge difference and should indeed be taken in consideration. Is this all due to the version where I added pagination with arrow-key or do the added severity labels also have an impact? What did you use to track performace? edit: I see, just browser dev tools.
I would advocate for keeping the comments in the JSX files.
@moorereason I think my agent was a bit to aggresive when I asked it to "remove the comments I just added in the last commit". A comment that says: "function for left arrow key" while the function is called "leftarrowkey()" is not necessary, while others might.
I changed the PR to draft as I don't have time this week to address your valid points. Feel free to contribute of course.
It took some work to enhance performance, but I think we are there.
So I started with a report from before I added my changes and measured the performance:
As you can see, opening a resultDialog takes around 30ms which is almost instant.
After I added the feature to use arrowKeys to move to the next resultInfoDialog, the performanced decreased and opening a dialog took 186ms. This is the current release (Maester 1.3.):
However, this contains the bug of opening 3 dialogs at the same time and the rendering after this function takes much longer. (not visible in this screenshot).
We then went on and I came with this PR:
I included some strange logic to calculate the previous and next resultInfoDialog and already loaded that into memory. This was an even bigger performance hit.
And now, with the current version of this PR:
It's still slower than the original version of the dialog, but that's because there is added functionality. We need to calculate what dialog comes next in order to be able to switch to the next dialog. That takes time.
Furthermore, I went on and looked into the rest of the report and managed to shave off 30% of the initial load time (1200ms to 860ms).
My Agent summarizes the changes like this:
Overview
This PR significantly improves the performance when opening and navigating between test result dialogs. We've identified and fixed multiple performance bottlenecks that were causing slowdowns, particularly when dealing with large test result sets.
Changes Made
1. Single Dialog Instance Pattern
- Before: Each row in the results table created three separate instances of ResultInfoDialog (for the clickable ID, Title, and button).
- After: Implemented a single shared dialog instance that displays the currently selected item.
- Why: Drastically reduces DOM size and memory usage. For a table with 300 test results, we've gone from 900 dialog component instances to just 1.
2. React Optimization Techniques
- Added React.memo to prevent unnecessary re-renders of the dialog component
- Implemented useCallback for event handlers to maintain referential equality
- Optimized useMemo implementation for filtered/sorted data to prevent redundant calculations
- Removed unnecessary global dialog state manager that was causing state synchronization issues
3. Lazy Loading Implementation
- Added React.lazy and Suspense to defer loading the ResultInfoDialog component until needed
- The dialog component is now only mounted when a user interacts with a result, rather than on initial page load
4. Dialog Content Cleanup
- Ensured all dialog content is properly unmounted when closed (Tremor's Dialog component has unmount=true)
- Simplified state management to reduce overhead when opening/closing dialogs
Performance Impact
- Reduced Initial Rendering Time: By avoiding the creation of hundreds of dialog instances
- Faster Dialog Navigation: Optimized navigation between results with memoized handlers
- Lower Memory Usage: Through proper component cleanup and single instance pattern
- Smoother UI Experience: Eliminated lag when opening dialogs in large test result sets
Technical Notes
- All optimizations focus on runtime performance rather than build size > Still around the same size in bytes.
- No functionality was changed or removed, only performance optimizations
Testing
The changes have been tested with large result sets (300+ results) and show significant performance improvement when opening and navigating between dialogs.
@merill This one is now ready for review. Could you please have a look and accept the PR?