ref(replay): Refactor ReplayCurrentURL & CurrentScreen components #102951
Conversation
github-actions
bot
added
the
Scope: Frontend
ryan953
changed the title
ryan953
force-pushed
the
ryan953/replay-url-compactselect
branch
from
5009e99 to
90c9d04
Compare
ryan953
changed the title
ryan953
changed the title
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Grid Layout Misalignment After Refactor
The ContextContainer grid layout expects three columns (grid-template-columns: 1fr max-content max-content) but after the refactoring only receives two children: ReplayCurrentLocation and ReplaySidebarToggleButton. Previously it had three children: the URL/screen component, BrowserOSIcons, and the sidebar toggle. This causes incorrect grid layout where the second child occupies the second column instead of the third, leaving the third column definition unused.
static/app/components/events/eventReplay/replayPreviewPlayer.tsx#L244-L251
sentry/static/app/components/events/eventReplay/replayPreviewPlayer.tsx
Lines 244 to 251 in fc1fa02
| <ContextContainer> | ||
| {isVideoReplay ? <ReplayCurrentScreen /> : <ReplayCurrentUrl />} | ||
| <BrowserOSIcons /> | ||
| <ReplayCurrentLocation isLoading={false} /> |
There was a problem hiding this comment.
Bug: Replay Fullscreen Layout Breaks
The ReplayCurrentLocation component now needs the BrowserOSIcons and ReplayViewScale components to render properly, but these only work correctly when not in fullscreen mode. In fullscreen mode within the preview player, ReplayCurrentLocation renders these components, but they were previously rendered standalone. The ContextContainer grid layout (with grid-template-columns: 1fr max-content max-content) expects three children but ReplayCurrentLocation returns a Flex with potentially different alignment, breaking the expected layout when fullscreen.
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| import * as Sentry from '@sentry/react'; | ||
|
|
||
| import {Flex} from '@sentry/scraps/layout/flex'; | ||
| import {ExternalLink, Link} from '@sentry/scraps/link'; |
There was a problem hiding this comment.
Bug: Incorrect import path for Link component, causing a module import failure.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The Link component is incorrectly imported from '@sentry/scraps/link' instead of '@sentry/scraps/link/link'. This will lead to a module import failure at runtime because Link is not exported from the former path, preventing the replayCurrentLocationInput.tsx component from rendering and breaking the replay player view.
💡 Suggested Fix
Update the import statements to import {ExternalLink} from '@sentry/scraps/link'; and import {Link} from '@sentry/scraps/link/link'; to correctly resolve the Link component.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/replays/replayCurrentLocationInput.tsx#L6
Potential issue: The `Link` component is incorrectly imported from
`'@sentry/scraps/link'` instead of `'@sentry/scraps/link/link'`. This will lead to a
module import failure at runtime because `Link` is not exported from the former path,
preventing the `replayCurrentLocationInput.tsx` component from rendering and breaking
the replay player view.
Did we get this right? 👍 / 👎 to inform future reviews.
| <Flex gap="sm" flex="1" align="center"> | ||
| <Tooltip title={scrubbingTooltip} disabled={!scrubbingTooltip} skipWrapper> | ||
| <FlexTextCopyInput aria-label={t('Copy to clipboard')} size="sm"> | ||
| {currentLocation} | ||
| </FlexTextCopyInput> |
There was a problem hiding this comment.
The aria-label for the TextCopyInput is set to t('Copy to clipboard'), which is generic and doesn't describe the actual content (e.g., 'Current URL' or 'Current Screen Name'). For better accessibility, especially for screen readers, consider making the aria-label more descriptive of what URL/location is being displayed (e.g., 'Current replay location'). This would help users understand what information they're about to copy.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/replays/replayCurrentLocationInput.tsx#L68-L72
Potential issue: The `aria-label` for the TextCopyInput is set to `t('Copy to
clipboard')`, which is generic and doesn't describe the actual content (e.g., 'Current
URL' or 'Current Screen Name'). For better accessibility, especially for screen readers,
consider making the aria-label more descriptive of what URL/location is being displayed
(e.g., 'Current replay location'). This would help users understand what information
they're about to copy.
Did we get this right? 👍 / 👎 to inform future reviews.
| <ErrorBoundary customComponent={FatalIconTooltip}> | ||
| <BrowserOSIcons showBrowser={!isVideoReplay} isLoading={isLoading} /> | ||
| </ErrorBoundary> | ||
| <ErrorBoundary customComponent={FatalIconTooltip}> | ||
| <ReplayViewScale isLoading={isLoading} /> | ||
| </ErrorBoundary> |
There was a problem hiding this comment.
Both BrowserOSIcons and ReplayViewScale are wrapped with ErrorBoundary using the FatalIconTooltip error component. While this handles errors gracefully, ensure that if an error occurs in either component, the user still has a functional replay player. The FatalIconTooltip displays only an icon, which may not be obvious to users unfamiliar with the Sentry UI. Consider logging to Sentry (via ErrorBoundary integration) to ensure these errors are tracked.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/replays/replayCurrentLocation.tsx#L22-L27
Potential issue: Both `BrowserOSIcons` and `ReplayViewScale` are wrapped with
`ErrorBoundary` using the `FatalIconTooltip` error component. While this handles errors
gracefully, ensure that if an error occurs in either component, the user still has a
functional replay player. The `FatalIconTooltip` displays only an icon, which may not be
obvious to users unfamiliar with the Sentry UI. Consider logging to Sentry (via
`ErrorBoundary` integration) to ensure these errors are tracked.
Did we get this right? 👍 / 👎 to inform future reviews.
| <PlayerBreadcrumbContainer> | ||
| <PlayerContainer> | ||
| <ContextContainer> | ||
| {isLoading ? ( | ||
| <TextCopyInput size="sm" disabled> | ||
| {''} | ||
| </TextCopyInput> | ||
| ) : isVideoReplay ? ( | ||
| <ScreenNameContainer> | ||
| {replay?.getReplay()?.sdk.name?.includes('flutter') ? ( | ||
| <QuestionTooltip | ||
| isHoverable | ||
| title={tct( | ||
| 'In order to see the correct screen name, you need to configure the [link:Sentry Routing Instrumentation].', | ||
| { | ||
| link: ( | ||
| <ExternalLink href="https://docs.sentry.io/platforms/dart/guides/flutter/integrations/routing-instrumentation/" /> | ||
| ), | ||
| } | ||
| )} | ||
| size="sm" | ||
| /> | ||
| ) : null} | ||
| <ScreenNameInputContainer> | ||
| <ReplayCurrentScreen /> | ||
| </ScreenNameInputContainer> | ||
| </ScreenNameContainer> | ||
| ) : ( | ||
| <ReplayCurrentUrl /> | ||
| )} | ||
|
|
||
| <ErrorBoundary customComponent={FatalIconTooltip}> | ||
| <BrowserOSIcons showBrowser={!isVideoReplay} isLoading={isLoading} /> | ||
| </ErrorBoundary> | ||
| <ErrorBoundary customComponent={FatalIconTooltip}> | ||
| <ReplayViewScale isLoading={isLoading} /> | ||
| </ErrorBoundary> | ||
| <Flex gap="lg" flex="1"> | ||
| <ReplayCurrentLocation isLoading={isLoading} /> | ||
| {isFullscreen ? ( | ||
| <ReplaySidebarToggleButton | ||
| isOpen={isSidebarOpen} | ||
| setIsOpen={setIsSidebarOpen} | ||
| /> | ||
| ) : null} | ||
| </ContextContainer> | ||
| </Flex> |
There was a problem hiding this comment.
The old ContextContainer styled div has been replaced with a Flex component using gap="lg" for spacing. However, the old ContextContainer used grid-template-columns: 1fr max-content; (which is different from the new Flex layout). Verify that the new layout produces the same visual result across different screen sizes and content widths. The Flex component with gap="lg" may behave differently when items need to wrap or when space is constrained.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/replays/replayView.tsx#L41-L51
Potential issue: The old `ContextContainer` styled div has been replaced with a `Flex`
component using `gap="lg"` for spacing. However, the old `ContextContainer` used
`grid-template-columns: 1fr max-content;` (which is different from the new `Flex`
layout). Verify that the new layout produces the same visual result across different
screen sizes and content widths. The `Flex` component with `gap="lg"` may behave
differently when items need to wrap or when space is constrained.
Did we get this right? 👍 / 👎 to inform future reviews.
| } | ||
|
|
||
| function getCurrentLocation(replay: null | ReplayReader, currentTime: number) { | ||
| try { | ||
| return replay?.isVideoReplay() | ||
| ? getCurrentScreenName( | ||
| replay?.getReplay(), | ||
| replay?.getMobileNavigationFrames(), | ||
| currentTime | ||
| ) | ||
| : getCurrentUrl(replay?.getReplay(), replay?.getNavigationFrames(), currentTime); | ||
| } catch (error) { | ||
| Sentry.captureException(error); |
There was a problem hiding this comment.
The function getCurrentLocation() catches all errors with catch (error) and logs them to Sentry via Sentry.captureException(error). This is good for monitoring, but returning an empty string on error may silently hide issues from the user. Consider providing user feedback (e.g., a toast notification or inline error message) when this error occurs, so users know why they're seeing an empty location field.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/replays/replayCurrentLocationInput.tsx#L77-L89
Potential issue: The function `getCurrentLocation()` catches all errors with `catch
(error)` and logs them to Sentry via `Sentry.captureException(error)`. This is good for
monitoring, but returning an empty string on error may silently hide issues from the
user. Consider providing user feedback (e.g., a toast notification or inline error
message) when this error occurs, so users know why they're seeing an empty location
field.
Did we get this right? 👍 / 👎 to inform future reviews.
| <Tooltip title={scrubbingTooltip} disabled={!scrubbingTooltip} skipWrapper> | ||
| <FlexTextCopyInput aria-label={t('Current Location')} size="sm"> | ||
| {currentLocation} | ||
| </FlexTextCopyInput> |
There was a problem hiding this comment.
Bug: Interactive Empty Fields Bug
The TextCopyInput is missing the disabled prop when currentLocation is empty. Previously, both replayCurrentUrl.tsx and replayCurrentScreen.tsx rendered a disabled input when there was no data to display, but the refactored ReplayCurrentLocationInput component always renders an enabled input, even with empty content. This allows interaction with an empty field where none should be possible.
srest2021
left a comment
srest2021
left a comment
There was a problem hiding this comment.
Looks good, just have some minor UI suggestions
| } | ||
|
|
||
| const FlexTextCopyInput = styled(TextCopyInput)` | ||
| flex: 1; |
There was a problem hiding this comment.
Could just wrap <TextCopyInput> in <Flex flex="1"> instead. Low logaf though
|
|
||
| return ( | ||
| <Flex gap="sm" flex="1" align="center"> | ||
| <Tooltip title={scrubbingTooltip} disabled={!scrubbingTooltip} skipWrapper> |
There was a problem hiding this comment.
We can do isHoverable for this tooltip, otherwise it disappears when I go to click the links
| <Tooltip title={scrubbingTooltip} disabled={!scrubbingTooltip} skipWrapper> | |
| <Tooltip title={scrubbingTooltip} disabled={!scrubbingTooltip} skipWrapper isHoverable> |
| <Flex gap="sm" flex="1" align="center"> | ||
| <Tooltip title={scrubbingTooltip} disabled={!scrubbingTooltip} skipWrapper> | ||
| <FlexTextCopyInput aria-label={t('Current Location')} size="sm"> | ||
| {currentLocation} |
There was a problem hiding this comment.
(Cursor bot already mentioned this but) previously we were disabling TextCopyInput when the url is empty, do we still want to do this? Would probably also be safer to always display an empty string whenever isLoading is true in replayCurrentLocation.tsx.
| const isVideoReplay = replay?.isVideoReplay(); | ||
|
|
||
| return ( | ||
| <Flex align="center" flex="1" gap="sm" justify="between" radius="md"> |
There was a problem hiding this comment.
Pretty minor but there's a small spacing difference:
Before:
After:
Changing gap="sm" to gap="lg" gets it back to the way it was before
| <Flex align="center" flex="1" gap="sm" justify="between" radius="md"> | |
| <Flex align="center" flex="1" gap="lg" justify="between" radius="md"> |
This consolidates a bunch of stuff that was split up for video replays vs. non-video. We didn't need to split so much up into separate bits. Not that things are together it's easier to drop this component into
replayView.tsxas well asreplayPreviewPlayer.tsxThe flutter setup warning is really the only thing i moved around. It's on the right now with a more appropriatly sized icon: