Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded virtualization to replay breadcrumbs; switched many replay/globe components to shallow Zustand selectors; memoized callbacks and wrapped controls with memo; refactored rrweb player hook to use refs/playerRef for dimension updates and removed setActivityPeriods from the hook's store destructuring. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Breadcrumbs as "ReplayBreadcrumbs"
participant Store as "ReplayStore"
participant PlayerComp as "ReplayPlayer (component)"
participant RRWeb as "rrweb-player"
User->>Breadcrumbs: click event or group click (time)
Breadcrumbs->>Store: setCurrentTime(time)
Breadcrumbs->>PlayerComp: request goto(time)
PlayerComp->>RRWeb: rrweb.player.goto(time)
RRWeb-->>PlayerComp: playback position updated
PlayerComp->>Store: setCurrentTime(updated)
Store-->>Breadcrumbs: currentTime change (shallow subs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ow state selection with Zustand for improved performance and reactivity. Update multiple components including GlobePage, TimelineScrubber, ReplayCard, and others to enhance data handling efficiency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/replay/player/ReplayPlayerTopbar.tsx`:
- Around line 45-51: The filter is comparing event.type to the number 4 but
upstream events may have type as a string, so normalize/coerce event.type before
the comparison in ReplayPlayerTopbar (the loop over data.events that builds
transitions using firstTimestamp and pushes into transitions). Update the check
to coerce event.type (e.g., Number(event.type) or parseInt(event.type as string,
10)) and then compare to 4, keeping the existing event.data?.href guard so
pageTransitions are populated correctly.
In `@client/src/components/replay/ReplayBreadcrumbs.tsx`:
- Around line 243-253: handleEventClick is writing seconds into the shared
currentTime store while player.goto, slider math, and topbar transitions expect
milliseconds; change the setCurrentTime call to use milliseconds (timeInMs)
instead of timeInSeconds so the shared currentTime stays in ms. Locate
handleEventClick (uses firstTimestamp, player.goto, setCurrentTime) and replace
setCurrentTime(timeInSeconds) with setCurrentTime(timeInMs) so all consumers
remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 698e78eb-9244-4f5a-a561-9688ba9fed0c
📒 Files selected for processing (5)
client/src/components/replay/ReplayBreadcrumbs.tsxclient/src/components/replay/player/ReplayPlayer.tsxclient/src/components/replay/player/ReplayPlayerControls.tsxclient/src/components/replay/player/ReplayPlayerTopbar.tsxclient/src/components/replay/player/hooks/useReplayPlayer.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/src/components/replay/player/ReplayPlayerTopbar.tsx (1)
48-55:⚠️ Potential issue | 🟠 MajorCoerce
event.typebefore comparing to avoid type mismatch.Line 49 compares
event.type === 4using strict equality with a number. However, based on the codebase patterns (seeReplayBreadcrumbs.tsxwhich usesString(event.type)before comparing to"4"), event types may arrive as strings from the server. This would leavepageTransitionsempty and break the URL tracking.🔧 Suggested fix
for (const event of data.events) { - if (event.type === 4 && event.data?.href) { + if (Number(event.type) === 4 && event.data?.href) { transitions.push({ time: event.timestamp - firstTimestamp, url: event.data.href, }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/replay/player/ReplayPlayerTopbar.tsx` around lines 48 - 55, The loop over data.events checks event.type with strict numeric equality which fails if the server sends it as a string; update the comparison in the loop that builds transitions (the block that iterates over data.events and pushes into transitions using firstTimestamp, event.timestamp and event.data?.href) to coerce event.type before comparing (e.g., use String(event.type) === "4" or Number(event.type) === 4) so page transitions are detected reliably regardless of incoming type.
🧹 Nitpick comments (6)
client/src/app/[site]/globe/3d/hooks/timelineLayer/useTimelineSessions.ts (1)
9-9: Consider moving this import to the external imports group.Per coding guidelines, external package imports should be grouped before internal imports. Move this import after line 3 (with the other external imports from
@tanstack/react-query,luxon, andreact).📦 Suggested import reordering
import { useQuery } from "@tanstack/react-query"; import { DateTime } from "luxon"; import { useEffect, useMemo } from "react"; +import { useShallow } from "zustand/react/shallow"; import { fetchSessions, GetSessionsResponse, SessionsParams } from "../../../../../../api/analytics/endpoints"; import { APIResponse } from "../../../../../../api/types"; import { authedFetch, buildApiParams } from "../../../../../../api/utils"; import { getFilteredFilters, useStore } from "../../../../../../lib/store"; import { SESSION_PAGE_FILTERS } from "../../../../../../lib/filterGroups"; -import { useShallow } from "zustand/react/shallow"; import { useTimelineStore } from "../../../timelineStore";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/`[site]/globe/3d/hooks/timelineLayer/useTimelineSessions.ts at line 9, The import "import { useShallow } from 'zustand/react/shallow'" is currently in the internal imports group; move it into the external imports group alongside other third-party imports (e.g., the `@tanstack/react-query`, luxon, and react imports) so external packages are grouped first, keeping import ordering consistent with project guidelines and keeping the import line "useShallow" adjacent to other external dependencies.client/src/app/[site]/globe/page.tsx (1)
26-26: MoveuseShallowinto the external import block.Line 26 places a third-party import after internal relative imports, so the file no longer follows the repo’s import grouping rule. As per coding guidelines, "Organize imports with external packages first, then internal imports; sort alphabetically within groups."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/`[site]/globe/page.tsx at line 26, The import of the third-party hook useShallow should be moved into the external imports block at the top of client/src/app/[site]/globe/page.tsx so external packages are grouped before internal relative imports; locate the current import statement "import { useShallow } from 'zustand/react/shallow';" and relocate it into the external imports section, maintaining alphabetical order among external imports and preserving existing internal relative imports order.client/src/components/replay/player/ReplayPlayerTopbar.tsx (1)
1-12: Import order inconsistency.
useShallowfromzustand/react/shallow(line 11) is an external package import but appears after internal imports. As per coding guidelines, organize imports with external packages first, then internal imports.♻️ Suggested reordering
import Link from "next/link"; import { useParams } from "next/navigation"; import { useMemo } from "react"; +import { useShallow } from "zustand/react/shallow"; import { useGetSessionReplayEvents } from "@/api/analytics/hooks/sessionReplay/useGetSessionReplayEvents"; import { BrowserTooltipIcon, CountryFlagTooltipIcon, DeviceTypeTooltipIcon, OperatingSystemTooltipIcon, } from "@/components/TooltipIcons/TooltipIcons"; -import { useShallow } from "zustand/react/shallow"; import { useReplayStore } from "../replayStore";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/replay/player/ReplayPlayerTopbar.tsx` around lines 1 - 12, Imports are out of order: move external package imports (e.g., Link from "next/link", useParams from "next/navigation", useMemo from "react", useShallow from "zustand/react/shallow") above internal project imports (e.g., useGetSessionReplayEvents, BrowserTooltipIcon/CountryFlagTooltipIcon/DeviceTypeTooltipIcon/OperatingSystemTooltipIcon, useReplayStore) so that external dependencies appear first, then internal modules; reorder the import block accordingly to satisfy the project's import ordering guideline.client/src/app/[site]/replay/components/ReplayList.tsx (1)
9-10: KeepuseShallowin the external import group.
zustand/react/shallowis a package import, so it should sit with the other third-party imports rather than below the local modules.As per coding guidelines, "Organize imports with external packages first, then internal imports; sort alphabetically within groups".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/`[site]/replay/components/ReplayList.tsx around lines 9 - 10, Move the external import "useShallow" from "zustand/react/shallow" into the top external-import group above internal imports; specifically, place the line importing useShallow before the local import of useReplayStore (from "@/components/replay/replayStore") and ensure imports are alphabetized within the external and internal groups so "useShallow" sits with other third-party imports.client/src/app/[site]/replay/components/ReplayCard.tsx (1)
31-32: KeepuseShallowwith the external imports.This adds a package import inside the internal import block. Move it up with the other third-party imports.
As per coding guidelines, "Organize imports with external packages first, then internal imports; sort alphabetically within groups".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/app/`[site]/replay/components/ReplayCard.tsx around lines 31 - 32, The import for useShallow is currently placed with internal imports; move the import statement "useShallow" from "zustand/react/shallow" into the external/third-party import block above the internal imports (where other external packages are listed) and ensure external imports are alphabetized; keep the internal import "useReplayStore" (from "@/components/replay/replayStore") in the internal group below. This will restore the external-first, alphabetized import order in ReplayCard.tsx.client/src/components/replay/player/hooks/useReplayPlayer.ts (1)
15-23: Type the player handle instead of usingany.The new resize path now depends on
$set()andtriggerResize(), souseRef<any>gives up the compile-time checks this refactor needs most. I’d give the ref a narrow player interface and mirror that type onnewPlayeras well.Please verify the exact `rrweb-player` instance surface exposed by the installed version before locking the interface. As per coding guidelines, "Use TypeScript with strict typing throughout both client and server".♻️ Suggested typing shape
+interface ReplayPlayerHandle { + addEventListener: (name: string, listener: (event: { payload: unknown }) => void) => void; + pause: () => void; + play: () => void; + getMetaData: () => { totalTime: number; isPlaying?: boolean }; + $set: (props: { width: number; height: number }) => void; + triggerResize: () => void; +} + export const useReplayPlayer = ({ data, width, height }: UseReplayPlayerProps) => { const playerContainerRef = useRef<HTMLDivElement>(null); - const playerRef = useRef<any>(null); + const playerRef = useRef<ReplayPlayerHandle | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/replay/player/hooks/useReplayPlayer.ts` around lines 15 - 23, Replace the untyped playerRef and newPlayer uses with a narrow TypeScript interface describing only the rrweb-player surface your code uses (e.g., methods like $set, triggerResize, play/pause, getCurrentTime/setCurrentTime, getDuration or other members referenced in useReplayPlayer). Change playerRef from useRef<any>(null) to useRef<PlayerHandle | null>(null) and type the newPlayer variable as PlayerHandle when creating the player, then update any call sites to respect the nullable ref. Confirm the exact method names/signatures against the installed rrweb-player version and keep the interface minimal to cover only the functions used by useReplayPlayer (playerRef, newPlayer, $set, triggerResize, and any time/control methods).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/src/components/replay/player/hooks/useReplayPlayer.ts`:
- Around line 46-48: Derived player height can go negative when
heightRef.current is 0; compute a clamped height (e.g., max(0, heightRef.current
- CONTROLS_HEIGHT)) and use that value instead of heightRef.current -
CONTROLS_HEIGHT in the player initialization and any subsequent $set() calls.
Update the code paths that read widthRef.current and heightRef.current
(including the initialization and the places referenced by the $set() call) to
use the clampedHeight variable so you never pass a negative height into the
player.
---
Duplicate comments:
In `@client/src/components/replay/player/ReplayPlayerTopbar.tsx`:
- Around line 48-55: The loop over data.events checks event.type with strict
numeric equality which fails if the server sends it as a string; update the
comparison in the loop that builds transitions (the block that iterates over
data.events and pushes into transitions using firstTimestamp, event.timestamp
and event.data?.href) to coerce event.type before comparing (e.g., use
String(event.type) === "4" or Number(event.type) === 4) so page transitions are
detected reliably regardless of incoming type.
---
Nitpick comments:
In `@client/src/app/`[site]/globe/3d/hooks/timelineLayer/useTimelineSessions.ts:
- Line 9: The import "import { useShallow } from 'zustand/react/shallow'" is
currently in the internal imports group; move it into the external imports group
alongside other third-party imports (e.g., the `@tanstack/react-query`, luxon, and
react imports) so external packages are grouped first, keeping import ordering
consistent with project guidelines and keeping the import line "useShallow"
adjacent to other external dependencies.
In `@client/src/app/`[site]/globe/page.tsx:
- Line 26: The import of the third-party hook useShallow should be moved into
the external imports block at the top of client/src/app/[site]/globe/page.tsx so
external packages are grouped before internal relative imports; locate the
current import statement "import { useShallow } from 'zustand/react/shallow';"
and relocate it into the external imports section, maintaining alphabetical
order among external imports and preserving existing internal relative imports
order.
In `@client/src/app/`[site]/replay/components/ReplayCard.tsx:
- Around line 31-32: The import for useShallow is currently placed with internal
imports; move the import statement "useShallow" from "zustand/react/shallow"
into the external/third-party import block above the internal imports (where
other external packages are listed) and ensure external imports are
alphabetized; keep the internal import "useReplayStore" (from
"@/components/replay/replayStore") in the internal group below. This will
restore the external-first, alphabetized import order in ReplayCard.tsx.
In `@client/src/app/`[site]/replay/components/ReplayList.tsx:
- Around line 9-10: Move the external import "useShallow" from
"zustand/react/shallow" into the top external-import group above internal
imports; specifically, place the line importing useShallow before the local
import of useReplayStore (from "@/components/replay/replayStore") and ensure
imports are alphabetized within the external and internal groups so "useShallow"
sits with other third-party imports.
In `@client/src/components/replay/player/hooks/useReplayPlayer.ts`:
- Around line 15-23: Replace the untyped playerRef and newPlayer uses with a
narrow TypeScript interface describing only the rrweb-player surface your code
uses (e.g., methods like $set, triggerResize, play/pause,
getCurrentTime/setCurrentTime, getDuration or other members referenced in
useReplayPlayer). Change playerRef from useRef<any>(null) to useRef<PlayerHandle
| null>(null) and type the newPlayer variable as PlayerHandle when creating the
player, then update any call sites to respect the nullable ref. Confirm the
exact method names/signatures against the installed rrweb-player version and
keep the interface minimal to cover only the functions used by useReplayPlayer
(playerRef, newPlayer, $set, triggerResize, and any time/control methods).
In `@client/src/components/replay/player/ReplayPlayerTopbar.tsx`:
- Around line 1-12: Imports are out of order: move external package imports
(e.g., Link from "next/link", useParams from "next/navigation", useMemo from
"react", useShallow from "zustand/react/shallow") above internal project imports
(e.g., useGetSessionReplayEvents,
BrowserTooltipIcon/CountryFlagTooltipIcon/DeviceTypeTooltipIcon/OperatingSystemTooltipIcon,
useReplayStore) so that external dependencies appear first, then internal
modules; reorder the import block accordingly to satisfy the project's import
ordering guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffafd089-90c5-4d4f-b985-f5d468f28c59
📒 Files selected for processing (7)
client/src/app/[site]/globe/3d/hooks/timelineLayer/useTimelineSessions.tsclient/src/app/[site]/globe/components/TimelineScrubber.tsxclient/src/app/[site]/globe/page.tsxclient/src/app/[site]/replay/components/ReplayCard.tsxclient/src/app/[site]/replay/components/ReplayList.tsxclient/src/components/replay/player/ReplayPlayerTopbar.tsxclient/src/components/replay/player/hooks/useReplayPlayer.ts
Comment on lines
+46
to
+48
| width: widthRef.current, | ||
| // subtract for the custom controls | ||
| height: height - CONTROLS_HEIGHT, | ||
| height: heightRef.current - CONTROLS_HEIGHT, |
Contributor
There was a problem hiding this comment.
Clamp the derived player height before passing it through.
If height is 0 during initial measurement or a collapsing layout, this computes a negative value and feeds it into both initialization and $set().
🐛 Suggested guard
- height: heightRef.current - CONTROLS_HEIGHT,
+ height: Math.max(heightRef.current - CONTROLS_HEIGHT, 0),
...
- height: height - CONTROLS_HEIGHT,
+ height: Math.max(height - CONTROLS_HEIGHT, 0),Also applies to: 146-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/components/replay/player/hooks/useReplayPlayer.ts` around lines 46
- 48, Derived player height can go negative when heightRef.current is 0; compute
a clamped height (e.g., max(0, heightRef.current - CONTROLS_HEIGHT)) and use
that value instead of heightRef.current - CONTROLS_HEIGHT in the player
initialization and any subsequent $set() calls. Update the code paths that read
widthRef.current and heightRef.current (including the initialization and the
places referenced by the $set() call) to use the clampedHeight variable so you
never pass a negative height into the player.
…seconds directly for improved accuracy in seeking functionality.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Performance Improvements
New Features
Refactor