Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an issue where the first span was being auto-selected on mobile due to useIsMobile() initially returning false until the component mounted, causing desktop-only initialization logic to run on the first render.
Changes:
- Updated
useIsMobile()to exposeisMountedalongsideisMobileso callers can avoid acting on the pre-mount value. - Updated
TraceViewerandSimpleTraceViewerto bail out of the “auto-select first span” effect when!isMounted || isMobile. - Cleaned up
TraceViewerReact import usage (no longer relies onReact.useReffor initialization gating).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/ui/src/components/shared.ts |
Changes useIsMobile() to return { isMounted, isMobile } and preserves pre-mount isMobile as false. |
packages/ui/src/components/TraceViewer/TraceViewer.tsx |
Uses isMounted to prevent initial auto-selection from running before mobile detection is stable. |
packages/saas/src/components/SimpleTraceViewer.tsx |
Applies the same isMounted/isMobile guard to prevent unintended default selection on mobile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -157,5 +157,8 @@ export const useIsMobile = () => { | |||
| return () => mediaQuery.removeEventListener("change", handleChange); | |||
| }, []); | |||
|
|
|||
| return mounted ? isMobile : false; | |||
| return { | |||
| isMounted, | |||
| isMobile: isMounted ? isMobile : false, | |||
| }; | |||
There was a problem hiding this comment.
useIsMobile previously returned a boolean but now returns an object with { isMounted, isMobile }. Since this hook is re-exported from the UI package entrypoint, this is an API-breaking change and the name no longer matches the return type. Consider either keeping useIsMobile() returning a boolean and introducing a new hook (e.g. useIsMobileState() / useMobileInfo()) for the {isMounted, isMobile} shape, or renaming this hook to reflect the new contract and updating any public docs/changelog accordingly.
There was a problem hiding this comment.
Legit concern. Updated to a standalone useIsMounted() hook
0f772d7 to
dc8fb5c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| if (!hasInitialized.current) { | ||
| hasInitialized.current = true; | ||
| } | ||
| if (!isMounted || isMobile) return; | ||
|
|
||
| if (!isMobile && selectedTraceSpans.length > 0 && !selectedSpan) { | ||
| if (selectedTraceSpans.length > 0 && !selectedSpan) { | ||
| setSelectedSpan(selectedTraceSpans[0]); | ||
| } | ||
| }, [selectedTraceSpans, isMobile, selectedSpan]); | ||
| }, [selectedTraceSpans, isMobile, isMounted, selectedSpan]); |
There was a problem hiding this comment.
Gating the auto-selection effect behind a stateful useIsMounted() means span auto-selection can’t happen until after the “mounted” rerender, which adds an extra render cycle on desktop (mount rerender, then selection rerender). If you want to keep the behavior but avoid the extra render, consider skipping only the first effect run via a useRef flag instead of adding a mounted state dependency.
| useEffect(() => { | ||
| if (!isMobile && spans.length > 0 && !selectedSpan) { | ||
| if (!isMounted || isMobile) return; | ||
|
|
||
| if (spans.length > 0 && !selectedSpan) { | ||
| setSelectedSpan(spans[0]); | ||
| } | ||
| }, [spans, selectedSpan, isMobile]); | ||
| }, [spans, selectedSpan, isMobile, isMounted]); |
There was a problem hiding this comment.
Using useIsMounted() state to gate this auto-select effect delays selectedSpan initialization until after the mounted rerender, introducing an extra render on desktop. If that matters, consider skipping only the first effect run via a useRef flag (or similar) instead of relying on a mounted state update.
Problem
In the previous implementation in this useEffect in TraceViewer:
isMobilewould first befalseand on the next rendertrue.Because this is how we define it in
useIsMobile:Back in that effect in TraceViewer 👆 This resulted in
setSelectedSpanbeing called even on mobile, on that first render whenisMobilewas brieflyfalse.Solution
Expose
isMounted, along withisMobile.Update the hook in TraceViewer with: