-
Notifications
You must be signed in to change notification settings - Fork 164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Performance bug fix to emit marks for only first tab #3182
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3182 +/- ##
==========================================
- Coverage 96.40% 96.39% -0.02%
==========================================
Files 784 785 +1
Lines 22144 22154 +10
Branches 7191 7192 +1
==========================================
+ Hits 21349 21356 +7
- Misses 788 791 +3
Partials 7 7 ☔ View full report in Codecov by Sentry. |
The first enabled tab is not necessarily the tab that is initially open, because developers can control which tab they want to show initially using the For example, something roughly like: const [activeTabId, setActiveTabId] = useControllable(...);
const initialTabId = useState(activeTabId)[0];
const isOnInitialTab = useRef(true);
if(isOnInitialTab.current) {
isOnInitialTab.current = activeTabId === initialTabId;
}
...
<TabContext.Provider
value={{
isInInitialTab: isOnInitialTab.current,
}}
> |
@@ -64,6 +66,7 @@ export function usePerformanceMarks( | |||
} | |||
|
|||
const renderedMarkName = `${name}Rendered`; | |||
console.log('mark'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a left-over.
isInFirstTab: boolean | null; | ||
} | ||
export const TabContext = createContext<TabContextProps>({ | ||
isInFirstTab: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to change this to isInDefaultTab
or hasTabChanged
. First tab means the first one from the left and the default tab might not be that one.
const attributes = useDOMAttribute(elementRef, 'data-analytics-performance-mark', id); | ||
const evaluateComponentVisibility = useEvaluateComponentVisibility(); | ||
useEffect(() => { | ||
if (!enabled() || !elementRef.current || isInModal) { | ||
if (!enabled || !elementRef.current || isInModal || isInFirstTab === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have changed !enabled()
to enabled
. Is this on purpose?
The type of enabled
is still function.
enabled: () => boolean, |
Description
Bug - {component}Rendered and {component}Updated marks are emitted even if current component is adding to the second tab component. We don't want to publish metrics for the sections which are not visible on page load.
Fix - Updated the usePerformanceMarks logic to emit markers only if the primary button or table is added to the first tab.
Related links, issue #, if available: n/a
How has this been tested?
Locally tested the change
TODO - Updated unit and integration tests
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.