Feature/replay#302
Merged
Merged
Conversation
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/sdks/nextjs/index.tsx (2)
167-169:⚠️ Potential issue | 🟡 MinorMissing optional chaining on
decrementcall.The
decrementfunction useswindow.op()directly without optional chaining, unlike other similar functions (track,identify,increment, etc.) that usewindow.op?.(). This could cause a runtime error ifwindow.opis undefined.🐛 Proposed fix
function decrement(payload: DecrementPayload) { - window.op('decrement', payload); + window.op?.('decrement', payload); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/nextjs/index.tsx` around lines 167 - 169, The decrement function calls window.op without optional chaining and can throw if window.op is undefined; update the decrement function (named decrement and located alongside track, identify, increment) to call window.op using optional chaining like the other handlers (e.g., window.op?.(...)) so it safely no-ops when window.op is not present.
171-185:⚠️ Potential issue | 🟡 MinorMissing optional chaining on multiple functions.
Several functions (
fetchDeviceId,clearRevenue,pendingRevenue,revenue,flushRevenue) accesswindow.opproperties directly without optional chaining, which could cause runtime errors if the SDK hasn't loaded yet.🛡️ Proposed fix for defensive access
function fetchDeviceId() { - return window.op.fetchDeviceId(); + return window.op?.fetchDeviceId(); } function clearRevenue() { - window.op.clearRevenue(); + window.op?.clearRevenue(); } function pendingRevenue(amount: number, properties?: Record<string, unknown>) { - window.op.pendingRevenue(amount, properties); + window.op?.pendingRevenue(amount, properties); } function revenue(amount: number, properties?: Record<string, unknown>) { - return window.op.revenue(amount, properties); + return window.op?.revenue(amount, properties); } function flushRevenue() { - return window.op.flushRevenue(); + return window.op?.flushRevenue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/nextjs/index.tsx` around lines 171 - 185, The listed functions (fetchDeviceId, clearRevenue, pendingRevenue, revenue, flushRevenue) access window.op directly and should defensively use optional chaining to avoid runtime errors if the SDK isn't loaded; update each function to call window.op?.<methodName>? with safe returns (e.g., return undefined or a Promise if original returned one) and ensure pendingRevenue and revenue still accept parameters while guarding calls with window.op?.pendingRevenue?.(amount, properties) and window.op?.revenue?.(amount, properties); similarly guard fetchDeviceId, clearRevenue and flushRevenue with window.op?.fetchDeviceId?.(), window.op?.clearRevenue?.(), and window.op?.flushRevenue?.() so missing window.op or missing methods won’t throw.packages/sdks/sdk/src/index.ts (1)
177-188:⚠️ Potential issue | 🟡 MinorAvoid mutating the input parameter.
delete properties?.deviceIdmutates the caller's object, which can cause unexpected side effects for code that retains a reference to it. Destructure instead to avoid mutation.♻️ Proposed fix using destructuring
async revenue( amount: number, properties?: TrackProperties & { deviceId?: string } ) { - const deviceId = properties?.deviceId; - delete properties?.deviceId; + const { deviceId, ...restProperties } = properties ?? {}; return this.track('revenue', { - ...(properties ?? {}), + ...restProperties, ...(deviceId ? { __deviceId: deviceId } : {}), __revenue: amount, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/sdk/src/index.ts` around lines 177 - 188, The revenue method currently mutates the caller's properties by using delete properties?.deviceId; fix this by destructuring deviceId out of the incoming properties (e.g., const { deviceId, ...rest } = properties ?? {}) so you create a new object without mutating the original, then call this.track('revenue', { ...rest, ...(deviceId ? { __deviceId: deviceId } : {}), __revenue: amount }); update the revenue function to use the new local rest object instead of deleting from properties.
🟡 Minor comments (6)
apps/public/public/op1.bak.js-115-124 (1)
115-124:⚠️ Potential issue | 🟡 MinorMutating the input parameter can cause unexpected side effects.
delete i?.deviceIdmodifies the caller's object directly. If the caller reuses the options object,deviceIdwill be unexpectedly missing. Consider destructuring to avoid mutation.Suggested fix using destructuring
async revenue(e, i) { - const t = i?.deviceId; - return ( - delete i?.deviceId, - this.track('revenue', { - ...(i ?? {}), - ...(t ? { __deviceId: t } : {}), - __revenue: e, - }) - ); + const { deviceId: t, ...rest } = i ?? {}; + return this.track('revenue', { + ...rest, + ...(t ? { __deviceId: t } : {}), + __revenue: e, + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public/public/op1.bak.js` around lines 115 - 124, The revenue method is mutating the caller's options object by using "delete i?.deviceId"; instead, avoid side effects by destructuring the parameter (e.g., const { deviceId, ...rest } = i ?? {}) and then call this.track('revenue', { ...rest, ...(deviceId ? { __deviceId: deviceId } : {}), __revenue: e }); update the revenue function to use these local variables (deviceId/rest) so the original i is not modified.apps/public/public/op1.bak.js-1-337 (1)
1-337:⚠️ Potential issue | 🟡 MinorRemove or exclude the outdated
op1.bak.jsfile.This is a minified backup of an older version of the OpenPanel SDK. The file contains the same logic as
op1.jsbut with less aggressive minification. Sinceop1.jsis the production version being used, the.bak.jsbackup should either be removed from the repository or added to.gitignore. Keeping both adds maintenance burden without providing value.The minified variable names (
u,l,p, etc.) are expected in compiled/minified code and don't violate coding standards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public/public/op1.bak.js` around lines 1 - 337, Remove the outdated backup bundle op1.bak.js (duplicate of the production op1.js): either delete op1.bak.js from the repository and any build/deploy inputs that include it, or add op1.bak.js to .gitignore so it isn’t committed; ensure no runtime or build scripts reference op1.bak.js (look for usages of window.op / window.openpanel assignment in your build config or HTML includes) before committing the change.apps/start/src/modals/event-details.tsx-278-281 (1)
278-281:⚠️ Potential issue | 🟡 MinorReplace
as anycasts and fix falsy value filtering to follow type safety guidelines.Line 278–281: Change
item.value as anytoas unknown.Lines 298–304, 306–307: The pattern
item.value && (filterable as any)[item.name]has two issues:
item.value &&skips valid falsy values like0,false, or empty strings(filterable as any)bypasses type checking on an already properly typed objectUse a type guard to safely narrow the key type and a helper function to check for non-null/undefined values:
Proposed fix
+const hasFilterValue = (value: unknown) => + value !== null && value !== undefined; + +const isFilterableKey = ( + name: string, +): name is keyof typeof filterable => Object.hasOwn(filterable, name); <KeyValueGrid columns={1} data={properties} onItemClick={(item) => { popModal(); - setFilter(`properties.${item.name}`, item.value as any); + setFilter(`properties.${item.name}`, item.value as unknown); }} /> <KeyValueGrid columns={1} data={data} onItemClick={(item) => { - const isFilterable = item.value && (filterable as any)[item.name]; - if (isFilterable) { + const isFilterable = + hasFilterValue(item.value) && isFilterableKey(item.name); + if (isFilterable) { popModal(); setFilter(item.name as keyof IServiceEvent, item.value); } }} renderValue={(item) => { - const isFilterable = item.value && (filterable as any)[item.name]; + const isFilterable = + hasFilterValue(item.value) && isFilterableKey(item.name); if (isFilterable) { return (Per coding guidelines: prefer
unknownoverany, and leverage TypeScript's type narrowing instead of type assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/modals/event-details.tsx` around lines 278 - 281, Replace the unsafe casts and faulty falsy filtering in the event-details modal: change the cast in the onItemClick handler from item.value as any to item.value as unknown when calling setFilter (referencing popModal and setFilter), implement a type guard to narrow item.name to a valid key of the filterable object (used where you currently do (filterable as any)[item.name]) and remove the `item.value &&` guard; instead add a small helper (e.g., isPresent) that checks value !== null && value !== undefined so falsy-but-valid values (0, false, '') are kept, and use the type guard + isPresent to safely access and test (filterable)[item.name] without any casts.apps/start/src/components/events/table/columns.tsx-151-160 (1)
151-160:⚠️ Potential issue | 🟡 MinorExtract magic number
6to a named constant for clarity.Line 158 uses a hardcoded magic number
6for session ID preview length. Per coding guidelines, extract this to a meaningfully named constant.Proposed fix
cell({ row }) { const { sessionId } = row.original; + const SESSION_ID_PREVIEW_LENGTH = 6; return ( <ProjectLink href={`/sessions/${encodeURIComponent(sessionId)}`} className="whitespace-nowrap font-medium hover:underline" > - {sessionId.slice(0,6)} + {sessionId.slice(0, SESSION_ID_PREVIEW_LENGTH)} </ProjectLink> ); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/events/table/columns.tsx` around lines 151 - 160, Extract the hardcoded "6" used in the cell renderer for session preview into a named constant (e.g., SESSION_ID_PREVIEW_LENGTH) and use that constant in the slice call inside the cell({ row }) function where sessionId.slice(0,6) is used; place the constant near the top of the module or above the columns definition so it's clearly visible and update the ProjectLink display to use sessionId.slice(0, SESSION_ID_PREVIEW_LENGTH).apps/api/src/utils/ids.ts-76-78 (1)
76-78:⚠️ Potential issue | 🟡 MinorReplace
console.errorwith the project logger path.Use structured app logging here so this utility follows production logging standards and central observability.
As per coding guidelines "Remove
console.log,debugger, andalertstatements from production code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/utils/ids.ts` around lines 76 - 78, Replace the console.error call in the catch block in apps/api/src/utils/ids.ts with the centralized project logger: import the shared logger instance used across the app (e.g., logger or appLogger) at the top of the file and call logger.error with a structured message and the error object (include context like "GET /track/device-id" and any relevant identifiers). Ensure the new call uses structured fields (message, error) rather than console, so replace console.error('Error getting session end GET /track/device-id', error) with logger.error({ msg: 'Error getting session end GET /track/device-id', error }) or the project logger's equivalent.apps/start/src/components/sessions/replay/replay-timeline.tsx-109-137 (1)
109-137:⚠️ Potential issue | 🟡 MinorEvent grouping threshold uses stale width on container resize.
Line 116 reads
trackRef.current?.offsetWidthduring render, butgroupedEventsonly depends on[eventsWithOffset, duration]—not width. On window resize, fullscreen toggle, or responsive layout changes, the threshold stays based on the initial (or fallback 600) width, causing inaccurate grouping.Wrap width tracking with a ResizeObserver in
useEffect, store it in state, and add it to the dependency array:Proposed fix
+ const [trackWidth, setTrackWidth] = useState(0); + + useEffect(() => { + const node = trackRef.current; + if (!node) return; + const observer = new ResizeObserver(() => { + setTrackWidth(node.offsetWidth); + }); + setTrackWidth(node.offsetWidth); + observer.observe(node); + return () => observer.disconnect(); + }, []); const groupedEvents = useMemo(() => { if (!eventsWithOffset.length || duration <= 0) return []; @@ - const trackWidth = trackRef.current?.offsetWidth ?? 600; - const thresholdMs = (24 / trackWidth) * duration; + const effectiveTrackWidth = trackWidth || 600; + const thresholdMs = (24 / effectiveTrackWidth) * duration; @@ - }, [eventsWithOffset, duration]); + }, [eventsWithOffset, duration, trackWidth]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-timeline.tsx` around lines 109 - 137, The grouping threshold uses trackRef.current?.offsetWidth inside the useMemo for groupedEvents but width isn't in the dependency array, causing stale thresholds on resize; fix by tracking the container width in state (e.g., containerWidth) updated via a ResizeObserver in a useEffect that observes trackRef (create/cleanup observer), use containerWidth (fall back to 600) instead of reading trackRef directly, and add containerWidth to the useMemo deps so thresholdMs and grouping recompute when the container resizes; update symbols: groupedEvents, trackRef, thresholdMs, and the useMemo deps.
🧹 Nitpick comments (23)
apps/start/src/components/ui/scroll-area.tsx (1)
6-30: Consider migrating fromReact.forwardRefto ref-as-prop pattern for consistency.
VirtualScrollAreauses the legacyReact.forwardRefpattern, whileScrollArea(line 37) already uses the modern React 19+ ref-as-prop approach. For consistency and alignment with React 19+ conventions, consider refactoring this component.♻️ Proposed refactor to ref-as-prop pattern
-export const VirtualScrollArea = React.forwardRef< - HTMLDivElement, - { - children: React.ReactNode; - className?: string; - } ->(({ children, className }, ref) => { +export function VirtualScrollArea({ + children, + className, + ref, +}: { + children: React.ReactNode; + className?: string; + ref?: React.Ref<HTMLDivElement>; +}) { // The ref MUST point directly to the scrollable element // This element MUST have: // 1. overflow-y-auto (or overflow: auto) // 2. A constrained height (via flex-1 min-h-0 or fixed height) return ( <div ref={ref} className={cn('overflow-y-auto w-full', className)} style={{ // Ensure height is constrained by flex parent height: '100%', maxHeight: '100%', }} > {children} </div> ); -}); - -VirtualScrollArea.displayName = 'VirtualScrollArea'; +}As per coding guidelines: "Use ref as a prop instead of
React.forwardRefin React 19+".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/ui/scroll-area.tsx` around lines 6 - 30, VirtualScrollArea currently uses React.forwardRef; change it to the ref-as-prop pattern by converting the forwardRef wrapper into a normal functional component that accepts a typed ref prop (e.g. ref?: React.Ref<HTMLDivElement> or alternatively scrollRef?: React.Ref<HTMLDivElement> if you prefer avoiding the name collision), update the props type to include that ref prop alongside children and className, pass that prop directly to the div's ref attribute, and remove the React.forwardRef export; keep the existing className, style, and comments intact so the scrollable element still has overflow-y-auto and constrained height.apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.sessions.tsx (1)
7-11: Consider adding a loader for data prefetching.The route lacks a
loaderfunction to prefetch session data. Comparing to similar routes in the codebase (e.g., the session detail route), adding a loader would improve perceived performance by prefetching the initial page of sessions during navigation:export const Route = createFileRoute( '/_app/$organizationId/$projectId/profiles/$profileId/_tabs/sessions', )({ component: Component, loader: async ({ context, params }) => { await context.queryClient.prefetchInfiniteQuery( context.trpc.session.list.infiniteQueryOptions({ projectId: params.projectId, profileId: params.profileId, take: 50, search: '', }), ); }, });This is optional for infinite queries since users may be accustomed to seeing the loading state, but it would provide consistency with other routes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/routes/_app`.$organizationId.$projectId.profiles.$profileId._tabs.sessions.tsx around lines 7 - 11, The Route created by createFileRoute for the sessions tab (exported as Route with component Component) lacks a loader to prefetch the sessions list; add a loader property to the Route that calls context.queryClient.prefetchInfiniteQuery using context.trpc.session.list.infiniteQueryOptions with params.projectId and params.profileId (e.g., take: 50, search: ''), so the first page of sessions is prefetched during navigation and the component can render with cached data.apps/public/public/op1.bak.js (5)
139-150: Preferfor...ofover.forEach()for iterating the queue.As per coding guidelines,
for...ofloops are preferred over.forEach()and indexedforloops.Suggested refactor
flush() { - this.queue.forEach((e) => { - this.send({ + for (const e of this.queue) { + this.send({ ...e, payload: { ...e.payload, profileId: e.payload.profileId ?? this.profileId, }, }); - }), - (this.queue = []); + } + this.queue = []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public/public/op1.bak.js` around lines 139 - 150, The flush method currently uses this.queue.forEach(...) which violates the guideline preferring for...of; change flush to iterate with a for...of loop over this.queue (e.g., for (const e of this.queue) { this.send({ ...e, payload: { ...e.payload, profileId: e.payload.profileId ?? this.profileId } }); }) and then clear the queue (this.queue = []) after the loop, keeping the existing use of this.send and the profileId fallback logic intact.
28-28: Redundant null coalescing operator.When
iis truthy (which it must be to enter this branch),i ?? {}will always evaluate toi. The?? {}fallback is unreachable.Suggested simplification
- body: i ? JSON.stringify(i ?? {}) : void 0, + body: i ? JSON.stringify(i) : void 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public/public/op1.bak.js` at line 28, The ternary branch uses an unreachable null coalescing fallback: change the body expression from using JSON.stringify(i ?? {}) to JSON.stringify(i) when i is truthy; locate the assignment that contains "body: i ? JSON.stringify(i ?? {}) : void 0" and remove the "?? {}" so the branch becomes "body: i ? JSON.stringify(i) : void 0".
314-316: Preferfor...ofover.forEach()for processing the queue.As per coding guidelines,
for...ofloops are preferred over.forEach().Suggested refactor
- i.forEach((n) => { - n[0] in t && t[n[0]](...n.slice(1)); - }); + for (const n of i) { + if (n[0] in t) { + t[n[0]](...n.slice(1)); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public/public/op1.bak.js` around lines 314 - 316, Replace the i.forEach callback with a for...of loop to iterate the queue `i` (preserving the exact semantics): iterate using `for (const n of i)` and inside check `if (n[0] in t)` then call `t[n[0]](...n.slice(1))`; update the block containing `i.forEach((n) => { n[0] in t && t[n[0]](...n.slice(1)); });` to use this `for...of` pattern so `i`, `n`, and `t` references remain the same and behavior is unchanged.
173-175: Empty catch blocks silently swallow errors.Multiple empty catch blocks (lines 173, 209, 293, 306) hide failures without any logging or handling. While these may be intentional for non-critical operations (sessionStorage access, URL parsing), consider at minimum logging in debug mode to aid troubleshooting.
Example: Add debug logging to catch blocks
} catch { - this.pendingRevenues = []; + this.pendingRevenues = []; + this.log('Failed to load pending revenues from sessionStorage'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public/public/op1.bak.js` around lines 173 - 175, Empty catch blocks (e.g., the one swallowing errors before setting this.pendingRevenues) hide failures; update each catch to capture the error (catch (err)) and emit a minimal debug/log message (e.g., console.debug or a designated logger) including the error and context (mentioning the affected symbol like this.pendingRevenues, sessionStorage access, or URL parsing) and only suppress the error in production/non-debug modes—ensure you use the same pattern in all catch sites to aid troubleshooting without changing behavior in production.
318-323: Consider gating the console.warn behind debug mode.The
console.warnon invalid method calls is helpful for developers integrating the SDK, but may be noisy in production. Consider gating it behind the debug flag for consistency with thelogmethod.Suggested change
const a = t[n] ? t[n].bind(t) : void 0; typeof a == 'function' ? a(...r) - : console.warn(`OpenPanel: ${n} is not a function`); + : t.log(`${n} is not a function`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/public/public/op1.bak.js` around lines 318 - 323, The anonymous dispatcher (the arrow function that looks up a = t[n] and calls it) currently emits console.warn when a method name is invalid; change it to respect the module's debug flag and use the same logging mechanism as the existing log method instead of unconditionally calling console.warn. Specifically, replace the standalone console.warn(...) in the else branch with a conditional that checks the existing debug flag (the same flag used by the log method) and, when enabled, calls the central logger (the module's log function) or console.warn through that path, keeping the message `OpenPanel: ${n} is not a function`; ensure you reference the same context object (t) so the debug check and log invocation use the module instance's debug/log settings.apps/start/src/modals/event-details.tsx (1)
239-243: Use Next.jsImageand provide avatar alt text.Using raw
<img>here misses the project guideline and accessibility requirement for meaningful alt text.Proposed fix
+import Image from 'next/image'; {profile.avatar && ( - <img - className="size-4 rounded-full bg-border" - src={profile.avatar} - /> + <Image + alt={`${getProfileName(profile, false)} avatar`} + className="size-4 rounded-full bg-border" + height={16} + src={profile.avatar} + width={16} + /> )}As per coding guidelines, "Use proper image components (e.g., Next.js
<Image>) over<img>tags" and "provide meaningful alt text for images".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/modals/event-details.tsx` around lines 239 - 243, The code uses a raw <img> for profile.avatar which violates project guidelines and lacks accessibility alt text; replace the <img> with Next.js Image: import Image from "next/image" at the top, render <Image src={profile.avatar} alt={`${profile.name ?? "User"} avatar`} className="size-4 rounded-full bg-border" width={40} height={40} /> (adjust width/height to match "size-4"), or wrap Image in a container if responsive styling is needed, ensuring the profile.avatar value is a valid URL and preserving the original className styling..claude/notnow-settings.local.json (1)
1-28: Avoid committing broad local-agent permissions into the shared repo.This
.localconfig grants high-privilege capabilities (Bash,Edit,Write) plus multiple outbound fetch scopes. Prefer keeping this local-only (gitignored) or reducing to a least-privilege shared baseline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/notnow-settings.local.json around lines 1 - 28, The .local settings currently grant broad local-agent powers via the permissions.allow array (e.g., entries like "mcp__acp__Bash", "mcp__acp__Edit", "mcp__acp__Write" and many "WebFetch(domain:...)" scopes); remove or restrict these sensitive entries from the committed file and either (a) move this file out of the repo and add it to .gitignore, or (b) replace it with a sanitized template (e.g., .claude/notnow-settings.local.json.example) that contains a minimal least-privilege baseline in permissions.allow; ensure the live high-privilege scopes remain only in local developer copies and update docs to instruct developers to create their own ignored local config from the template.apps/worker/src/jobs/events.incoming-events.test.ts (1)
102-105: Move UUID regex to a top-level constant.Keeping the UUID pattern as a module-level
constavoids the inline suppression and makes expectations easier to reuse across tests.As per coding guidelines: "Use top-level regex literals instead of creating them in loops".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/jobs/events.incoming-events.test.ts` around lines 102 - 105, Extract the inline UUID regex used in the sessionId expectation (the expect.stringMatching(...) call) into a module-level constant (e.g., UUID_REGEX) and replace the inline pattern with that constant; remove the biome-ignore lint/performance/useTopLevelRegex comment, and update any other tests in the file that can reuse the new UUID_REGEX to keep expectations consistent and avoid creating regexes in-loop.packages/db/src/buffers/session-buffer.ts (1)
188-195: Extract cache TTL constant and type the public getter return.The duplicated
60 * 60is a magic value, andgetBufferSize()is clearer with an explicitPromise<number>return type.♻️ Suggested cleanup
+const SESSION_CACHE_TTL_SECONDS = 60 * 60; + - async add(event: IClickhouseEvent) { + async add(event: IClickhouseEvent) { @@ - 60 * 60 + SESSION_CACHE_TTL_SECONDS @@ - 60 * 60 + SESSION_CACHE_TTL_SECONDS @@ - getBufferSize() { + getBufferSize(): Promise<number> { return this.getBufferSizeWithCounter(() => this.redis.llen(this.redisKey)); }As per coding guidelines: "Use meaningful variable names instead of magic numbers - extract constants with descriptive names" and "Use explicit types for function parameters and return values when they enhance clarity".
Also applies to: 262-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/session-buffer.ts` around lines 188 - 195, Replace the duplicated magic TTL (60 * 60) used when setting session cache keys with a named constant (e.g., SESSION_CACHE_TTL_SECONDS) and use that constant in both places where multi.set is called for session cache; also add an explicit return type Promise<number> to the public getter getBufferSize() to improve clarity and typing—update the function signature of getBufferSize() and any related callers to respect the Promise<number> return. Ensure references to newSession.profile_id and multi.set(...) remain unchanged except for using the new constant.packages/db/src/buffers/replay-buffer.ts (1)
36-37: Add explicit return types for public methods.This class is a shared buffer primitive; explicit returns (
Promise<void>,Promise<number>) make contracts clearer at call sites.As per coding guidelines: "Use explicit types for function parameters and return values when they enhance clarity".
Also applies to: 55-56, 88-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/replay-buffer.ts` around lines 36 - 37, The public methods on the ReplayBuffer class (e.g., async add(chunk: IClickhouseSessionReplayChunk)) need explicit return types: annotate add as Promise<void> if it does not return a value, and annotate the other public methods in the same class with their appropriate Promise types (for example Promise<number> for size/count methods or Promise<void> for flush/clear methods); update each method signature in replay-buffer.ts to include the explicit Promise<> return type matching the actual return behavior so callers have a clear contract.packages/sdks/web/src/replay/index.ts (1)
1-2: Avoid introducing a barrel file inreplay/index.ts.This file is purely a re-export barrel; prefer importing from
./recorderdirectly from call sites or expose a non-barrel entrypoint.As per coding guidelines
**/index.{js,ts,jsx,tsx}: “Avoid barrel files (index files that re-export everything)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/replay/index.ts` around lines 1 - 2, Remove the barrel re-export in replay/index.ts: delete this file (or remove its export lines) and update all call sites to import startReplayRecorder, stopReplayRecorder, ReplayChunkPayload, and ReplayRecorderConfig directly from './recorder' instead of from the replay index; locate usages that import from the replay folder and change them to import { startReplayRecorder, stopReplayRecorder } from './recorder' and import type { ReplayChunkPayload, ReplayRecorderConfig } from './recorder'.packages/sdks/sdk/src/index.ts (1)
211-231: Unawaitedsend()calls may fail silently.The
flush()method fires offsend()calls without awaiting them. If any fail, errors are lost. Consider capturing the promises and logging failures, or returning them for the caller to handle.Also, the
for...ofloop aligns well with the coding guidelines.♻️ Optional: capture and handle errors from flushed sends
flush() { const remaining: TrackHandlerPayload[] = []; + const sendPromises: Promise<unknown>[] = []; for (const item of this.queue) { if (this.shouldQueue(item)) { remaining.push(item); continue; } const payload = item.type === 'replay' ? item.payload : { ...item.payload, profileId: 'profileId' in item.payload ? (item.payload.profileId ?? this.profileId) : this.profileId, }; - this.send({ ...item, payload } as TrackHandlerPayload); + sendPromises.push( + this.send({ ...item, payload } as TrackHandlerPayload).catch((err) => + this.log('Failed to flush payload', err) + ) + ); } this.queue = remaining; + return Promise.all(sendPromises); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/sdk/src/index.ts` around lines 211 - 231, The flush() method currently invokes send() without awaiting results, risking silent failures; update flush() (which iterates this.queue and calls this.send for items that shouldn't be queued) to collect the send() promises (e.g., into an array) and await them (preferably via Promise.allSettled) so failures can be detected and logged via the existing logger, and ensure any items that failed to send are left in this.queue (or re-queued) rather than being dropped; reference the flush, send, shouldQueue, queue, TrackHandlerPayload, and profileId symbols when making this change.apps/start/src/components/sessions/replay/replay-event-feed.tsx (1)
46-59: Effect dependency could be more precise.The effect's dependency array is
[visibleEvents.length], but the effect also readsvisibleEventsto check its length. While this works because you're comparing againstprevCountRef, ifvisibleEventsreference changes but length stays the same, the effect won't run (which is likely the desired behavior for performance).This is a subtle but intentional optimization. Consider adding a brief comment to clarify:
+ // Depend only on length to avoid re-running when events change but count doesn't }, [visibleEvents.length]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-event-feed.tsx` around lines 46 - 59, The effect in the ReplayEventFeed using useEffect (references viewportRef, visibleEvents, prevCountRef) intentionally depends only on visibleEvents.length for performance, but this intent isn’t documented; add a short inline comment above the useEffect explaining that the dependency is intentionally limited to visibleEvents.length so the effect only runs on length changes (not on reference changes) and that prevCountRef.current is used to detect new items for smooth scrolling, preserving the current behavior and optimization.apps/start/src/components/sessions/replay/replay-player.tsx (2)
153-155: Cleanup accessescontainerRef.currentwhich may be stale.In the cleanup function,
containerRef.currentis captured when the effect runs but may benullor point to a different element by the time cleanup executes (e.g., during unmount). The ref itself is stable, so accessing it at cleanup time is fine, but the current checkif (containerRef.current)should use the ref's value at cleanup time (which it does). However, if the component unmounts, the ref may already be cleared.This is typically safe but worth noting. A safer pattern would be to capture the container element at effect setup time:
♻️ Safer cleanup pattern
useEffect(() => { if (!events.length || !containerRef.current) return; + const container = containerRef.current; // Clear any previous player DOM - containerRef.current.innerHTML = ''; + container.innerHTML = ''; // ... rest of effect using `container` instead of `containerRef.current` return () => { // ... other cleanup - if (containerRef.current) { - containerRef.current.innerHTML = ''; - } + container.innerHTML = ''; // ... }; }, [/* deps */]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-player.tsx` around lines 153 - 155, The cleanup currently reads containerRef.current directly which can be cleared or changed before cleanup runs; capture the element at effect setup and use that captured reference in the cleanup instead: inside the effect (where containerRef is accessed) set const container = containerRef.current and then in the returned cleanup callback use container (e.g., if (container) container.innerHTML = '') rather than accessing containerRef.current there; this ensures the cleanup targets the same DOM node referenced by the effect and avoids stale/null ref races.
56-159: Consider the effect's dependency array completeness.The effect depends on
recordedDimensionswhich is derived fromeventsviauseMemo. Since botheventsandrecordedDimensionsare in the dependency array, changes toeventswill trigger the effect twice ifrecordedDimensionsalso changes. This is likely intentional but could cause the player to reinitialize unnecessarily.The current implementation should work correctly, but if you notice performance issues with frequent reinitializations, consider removing
recordedDimensionsfrom the dependency array since it's derived fromevents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-player.tsx` around lines 56 - 159, The effect useEffect(...) currently lists both events and recordedDimensions in its dependency array which causes double re-renders when recordedDimensions (derived from events) changes; to prevent unnecessary player reinitialization, remove recordedDimensions from the dependency array (keep events and the callbacks onPlayerReady/onPlayerDestroy and setters) or ensure recordedDimensions is memoized/stable before including it; update the dependency list referenced in the useEffect containing containerRef, playerRef, calcDimensions, and player lifecycle handling so the effect only reruns when events or true external inputs change.apps/start/src/components/sessions/replay/browser-chrome.tsx (1)
17-17: Clarify theurl !== falsecondition or adjust the type.The
urlprop is typed asReactNode, but the conditionurl !== falseon line 32 suggests an intent to usefalseas a special sentinel value to hide the URL bar. SinceReactNodeimplicitly allowsfalse(falsy JSX renders nothing), this works but may confuse readers.Consider either:
- Using
undefinedas the sentinel (more conventional):url !== undefinedor justurl &&- Updating the type to be explicit:
url?: ReactNode | false♻️ Option 1: Use undefined as sentinel
- {url !== false && ( + {url != null && ( <div className="flex-1 mx-4 px-3 h-8 py-1 text-sm bg-def-100 rounded-md border border-border flex items-center truncate"> {url} </div> )}Also applies to: 32-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/browser-chrome.tsx` at line 17, The prop `url` is treated as a false sentinel in the check `url !== false` but the prop type is only `ReactNode`; make the intent explicit by updating the prop signature to `url?: ReactNode | false` and keep the existing conditional (`url !== false`) where the URL bar is toggled (references: the `url` prop and the `url !== false` check in the render logic), so readers know `false` is an intentional sentinel value for hiding the URL bar.apps/start/src/components/sessions/replay/replay-utils.ts (1)
7-10: Simplify the Date type check.The
typeof event.createdAt === 'object'check is redundant when followed byinstanceof Date, sinceinstanceof Datealready implies it's an object and handles null (returns false).♻️ Suggested simplification
const t = - typeof event.createdAt === 'object' && event.createdAt instanceof Date + event.createdAt instanceof Date ? event.createdAt.getTime() : new Date(event.createdAt).getTime();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-utils.ts` around lines 7 - 10, Simplify the Date type check by removing the redundant typeof check: in the assignment to t (variable t in replay-utils.ts) use only "event.createdAt instanceof Date" to detect Date instances and call getTime(), otherwise fall back to new Date(event.createdAt).getTime(); update the conditional expression accordingly so it relies solely on instanceof Date.apps/start/src/types/rrweb-player.d.ts (1)
37-40: Consider narrowing theaddEventListenerhandler signature.The handler type
(...args: unknown[]) => voidis very loose. Based on the actual usage inreplay-player.tsx(lines 99-106), the handler receives an object with apayloadproperty. A more precise type would improve type safety:addEventListener?: ( event: string, - handler: (...args: unknown[]) => void, + handler: (e: { payload: unknown }) => void, ) => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/types/rrweb-player.d.ts` around lines 37 - 40, The addEventListener declaration uses a too-broad handler type; tighten it to reflect that handlers receive an object with a payload by changing the handler signature on addEventListener to accept a single parameter shaped like { payload: <specificType> } (or unknown if generic), e.g., addEventListener?: (event: string, handler: (evt: { payload: unknown }) => void) => void; update the type for the handler used in replay-player.tsx accordingly (replace the (...args: unknown[]) => void handler) so event handlers expect a single { payload } object.apps/start/src/components/sessions/replay/replay-context.tsx (1)
162-164: Use narrowing instead of a type assertion insetSpeed.Line 163 uses a cast to satisfy
includes, which weakens type-safety. A small type guard keeps this check explicit and assertion-free.💡 Proposed refactor
+const isSpeedOption = ( + value: number, +): value is (typeof SPEED_OPTIONS)[number] => { + return SPEED_OPTIONS.some((option) => option === value); +}; + const setSpeed = useCallback((s: number) => { - if (!SPEED_OPTIONS.includes(s as (typeof SPEED_OPTIONS)[number])) return; + if (!isSpeedOption(s)) return; playerRef.current?.setSpeed(s); }, []);As per coding guidelines, "Leverage TypeScript's type narrowing instead of type assertions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-context.tsx` around lines 162 - 164, The setSpeed function currently uses a type assertion to check membership in SPEED_OPTIONS which weakens type-safety; replace the assertion with a proper type guard (e.g., add an isSpeedOption(s: number): s is typeof SPEED_OPTIONS[number] function) and use that guard inside setSpeed to narrow the type before calling playerRef.current?.setSpeed(s), so the includes check is assertion-free and the compiler knows s is a valid speed option.apps/start/src/components/sessions/replay/replay-timeline.tsx (1)
26-39: Remove or complete the drag scaffolding.
isDragging,dragCleanupRef, andrafDragRefare currently unused in behavior, so Line 35-38 cleanup is effectively a no-op. Either implement drag lifecycle or remove the dead path to reduce maintenance noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-timeline.tsx` around lines 26 - 39, The unused drag scaffolding (isDragging, setIsDragging, dragCleanupRef, rafDragRef and the useEffect cleanup) should be either removed or completed: either delete the state/refs and the cleanup effect to eliminate dead code, or implement the drag lifecycle by wiring mousedown/touchstart to setIsDragging(true), adding mousemove/touchmove listeners that use rafDragRef for requestAnimationFrame updates and update hoverInfo, and store a cleanup function on dragCleanupRef that removes the listeners and cancels rafDragRef; ensure the existing useEffect cleanup calls dragCleanupRef.current() to remove listeners on unmount.apps/start/src/components/sessions/replay/index.tsx (1)
22-42: Avoid sorting all events on every 250ms URL update.Line 27-33 rebuilds and sorts the whole list whenever
currentTimechanges. Precompute sorted offsets once (onevents/startTime) and use binary search per tick to keep updates cheap.💡 Proposed refactor
+ const sortedWithOffset = useMemo(() => { + if (startTime == null || !events.length) return []; + return events + .map((event) => ({ event, offsetMs: getEventOffsetMs(event, startTime) })) + .filter(({ offsetMs }) => offsetMs >= -10_000) + .sort((a, b) => a.offsetMs - b.offsetMs); + }, [events, startTime]); + const currentUrl = useMemo(() => { - if (startTime == null || !events.length) { + if (!sortedWithOffset.length) { return ''; } - - const withOffset = events - .map((ev) => ({ - event: ev, - offsetMs: getEventOffsetMs(ev, startTime), - })) - .filter(({ offsetMs }) => offsetMs >= -10_000 && offsetMs <= currentTime) - .sort((a, b) => a.offsetMs - b.offsetMs); - - const latest = withOffset.at(-1); + let lo = 0; + let hi = sortedWithOffset.length; + while (lo < hi) { + const mid = (lo + hi) >>> 1; + if ((sortedWithOffset[mid]?.offsetMs ?? 0) <= currentTime) lo = mid + 1; + else hi = mid; + } + const latest = sortedWithOffset[lo - 1]; if (!latest) { return ''; } @@ - }, [events, currentTime, startTime]); + }, [currentTime, sortedWithOffset]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/index.tsx` around lines 22 - 42, Precompute and sort event offsets once instead of rebuilding on every currentTime tick: create a useMemo (dependent on events and startTime) that maps events to {offsetMs,event}, filters offsetMs >= -10_000, sorts by offsetMs and returns that sortedOffsets array; then change the currentUrl useMemo to only perform a binary search on sortedOffsets (using getEventOffsetMs-derived offsetMs stored in the precomputed entries) to find the latest entry with offsetMs <= currentTime and return its origin+path (falling back to ''/default path as before). Reference: useMemo that computes currentUrl, the withOffset logic, getEventOffsetMs, and variables events/startTime/currentTime.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (64)
.claude/CLAUDE.md.claude/notnow-settings.local.jsonapps/api/src/controllers/event.controller.tsapps/api/src/controllers/track.controller.tsapps/api/src/hooks/duplicate.hook.tsapps/api/src/hooks/request-logging.hook.tsapps/api/src/index.tsapps/api/src/routes/track.router.tsapps/api/src/utils/ids.tsapps/public/content/docs/(tracking)/sdks/astro.mdxapps/public/content/docs/(tracking)/sdks/nextjs.mdxapps/public/content/guides/nextjs-analytics.mdxapps/public/public/op1-replay.jsapps/public/public/op1.bak.jsapps/public/public/op1.jsapps/public/src/app/layout.tsxapps/start/package.jsonapps/start/src/components/events/table/columns.tsxapps/start/src/components/sessions/replay/browser-chrome.tsxapps/start/src/components/sessions/replay/index.tsxapps/start/src/components/sessions/replay/replay-context.tsxapps/start/src/components/sessions/replay/replay-controls.tsxapps/start/src/components/sessions/replay/replay-event-feed.tsxapps/start/src/components/sessions/replay/replay-event-item.tsxapps/start/src/components/sessions/replay/replay-player.tsxapps/start/src/components/sessions/replay/replay-timeline.tsxapps/start/src/components/sessions/replay/replay-utils.tsapps/start/src/components/sessions/table/columns.tsxapps/start/src/components/ui/scroll-area.tsxapps/start/src/hooks/use-page-tabs.tsapps/start/src/modals/event-details.tsxapps/start/src/routeTree.gen.tsapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.sessions.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.tsxapps/start/src/routes/_app.$organizationId.$projectId.sessions_.$sessionId.tsxapps/start/src/types/rrweb-player.d.tsapps/start/src/utils/op.tsapps/worker/src/boot-cron.tsapps/worker/src/jobs/cron.tsapps/worker/src/jobs/events.incoming-event.tsapps/worker/src/jobs/events.incoming-events.test.tsapps/worker/src/metrics.tsapps/worker/src/utils/session-handler.tspackages/common/src/id.tspackages/db/code-migrations/10-add-session-replay.tspackages/db/code-migrations/migrate.tspackages/db/src/buffers/index.tspackages/db/src/buffers/replay-buffer.tspackages/db/src/buffers/session-buffer.tspackages/db/src/clickhouse/client.tspackages/db/src/services/session.service.tspackages/queue/src/queues.tspackages/sdks/astro/src/OpenPanelComponent.astropackages/sdks/nextjs/index.tsxpackages/sdks/sdk/src/index.tspackages/sdks/web/package.jsonpackages/sdks/web/src/index.tspackages/sdks/web/src/replay/index.tspackages/sdks/web/src/replay/recorder.tspackages/sdks/web/tsup.config.tspackages/trpc/src/routers/session.tspackages/validation/src/track.validation.tstest.tstooling/publish/publish.ts
💤 Files with no reviewable changes (1)
- test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/worker/src/jobs/events.incoming-event.ts (1)
169-173:⚠️ Potential issue | 🟠 MajorUse incoming identifiers as fallback when no cached session exists.
In the server/past-event branch, Line 171 and Line 172 currently default to empty strings. If no session is found, this drops provided
deviceId/sessionIdand can orphan event-session linkage.🛠️ Suggested fix
const payload = { ...baseEvent, - deviceId: session?.device_id ?? '', - sessionId: session?.id ?? '', + deviceId: session?.device_id ?? deviceId, + sessionId: session?.id ?? sessionId, referrer: session?.referrer ?? undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/jobs/events.incoming-event.ts` around lines 169 - 173, The payload construction is overwriting incoming device/session IDs with empty strings when session is missing; update the deviceId and sessionId assignments in the payload to fall back to the incoming identifiers from baseEvent before defaulting to ''. Specifically, change the deviceId and sessionId expressions used when building payload (currently using session?.device_id ?? '' and session?.id ?? '') to session?.device_id ?? baseEvent.deviceId ?? '' and session?.id ?? baseEvent.sessionId ?? '' so the incoming IDs are preserved when no cached session exists.packages/sdks/nextjs/index.tsx (1)
168-177:⚠️ Potential issue | 🟡 MinorInconsistent optional chaining usage.
The
decrement,fetchDeviceId, andgetDeviceIdfunctions accesswindow.opwithout optional chaining (?.), while other functions liketrack,identify, andclearusewindow.op?.(). This inconsistency could cause runtime errors ifwindow.opis undefined.💡 Suggested fix
function decrement(payload: DecrementPayload) { - window.op('decrement', payload); + window.op?.('decrement', payload); } function fetchDeviceId() { - return window.op.fetchDeviceId(); + return window.op?.fetchDeviceId(); } function getDeviceId() { - return window.op.getDeviceId(); + return window.op?.getDeviceId(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/nextjs/index.tsx` around lines 168 - 177, The functions decrement, fetchDeviceId, and getDeviceId call window.op directly and should use optional chaining like the other functions to avoid runtime errors when window.op is undefined; update decrement (function decrement(payload: DecrementPayload)), fetchDeviceId (function fetchDeviceId()), and getDeviceId (function getDeviceId()) to call window.op?.decrement(...), window.op?.fetchDeviceId(), and window.op?.getDeviceId() respectively so calls are no-ops when window.op is missing.
♻️ Duplicate comments (10)
apps/worker/src/utils/session-handler.ts (1)
137-154:⚠️ Potential issue | 🟠 MajorLegacy fallback lookup should not require both IDs.
The condition on line 138 requires both
currentDeviceIdANDpreviousDeviceIdto be truthy. If only one ID is provided (non-empty), the legacy lookup is skipped entirely, potentially missing valid session-end jobs.The checks should be separated to run independently:
🔧 Proposed fix
- // TODO: Remove this when migrated to deviceId - if (args.currentDeviceId && args.previousDeviceId) { - // Check current device job - const currentJob = await sessionsQueue.getJob( - getSessionEndJobId(args.projectId, args.currentDeviceId), - ); - if (currentJob) { - return await handleJobStates(currentJob, args.currentDeviceId); - } - - // Check previous device job - const previousJob = await sessionsQueue.getJob( - getSessionEndJobId(args.projectId, args.previousDeviceId), - ); - if (previousJob) { - return await handleJobStates(previousJob, args.previousDeviceId); - } - } + // TODO: Remove this when migrated to deviceId + if (args.currentDeviceId) { + const currentJob = await sessionsQueue.getJob( + getSessionEndJobId(args.projectId, args.currentDeviceId), + ); + if (currentJob) { + return await handleJobStates(currentJob, args.currentDeviceId); + } + } + + if (args.previousDeviceId) { + const previousJob = await sessionsQueue.getJob( + getSessionEndJobId(args.projectId, args.previousDeviceId), + ); + if (previousJob) { + return await handleJobStates(previousJob, args.previousDeviceId); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/utils/session-handler.ts` around lines 137 - 154, The legacy fallback currently requires both args.currentDeviceId and args.previousDeviceId to be truthy before checking session-end jobs, which skips lookup when only one ID is present; update the logic in session-handler.ts to check each ID independently: if args.currentDeviceId is present, call sessionsQueue.getJob(getSessionEndJobId(args.projectId, args.currentDeviceId)) and pass any found job to handleJobStates(args.currentDeviceId); separately, if args.previousDeviceId is present, call sessionsQueue.getJob(getSessionEndJobId(args.projectId, args.previousDeviceId)) and pass any found job to handleJobStates(args.previousDeviceId); keep the same await/return behavior when a job is handled.packages/sdks/web/src/index.ts (1)
135-138:⚠️ Potential issue | 🟠 Major
__OPENPANEL_REPLAY_URL__is checked but never used in URL construction.The build-time injected constant is checked for existence at line 135, but line 137 never actually uses its value. The URL fallback chain should include
__OPENPANEL_REPLAY_URL__.💡 Suggested fix
if (typeof __OPENPANEL_REPLAY_URL__ !== 'undefined') { const scriptEl = document.currentScript as HTMLScriptElement | null; - const url = this.options.sessionReplay?.scriptUrl || scriptEl?.src?.replace('.js', '-replay.js') || 'https://openpanel.dev/op1-replay.js'; + const url = + this.options.sessionReplay?.scriptUrl ?? + __OPENPANEL_REPLAY_URL__ ?? + scriptEl?.src?.replace('.js', '-replay.js') ?? + 'https://openpanel.dev/op1-replay.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/index.ts` around lines 135 - 138, The code checks __OPENPANEL_REPLAY_URL__ but never uses it when constructing the replay script URL; update the url construction in the block that sets the url variable (using this.options.sessionReplay?.scriptUrl, document.currentScript?.src replacement, and the default) to include __OPENPANEL_REPLAY_URL__ as a higher-priority fallback (e.g., before the default) so the build-time injected constant is honored when present.apps/start/src/modals/event-details.tsx (1)
211-213:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the close icon button (still unresolved).
At Line 211-213, the icon-only button still has no accessible name; screen readers won’t announce its action.
As per coding guidelines, "Use semantic HTML and ARIA attributes for accessibility".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/modals/event-details.tsx` around lines 211 - 213, The Button with the XIcon in event-details.tsx (the onClick calling popModal) is icon-only and lacks an accessible name; add an accessible label (e.g., aria-label or title) to the Button component so screen readers announce its purpose (for example aria-label="Close" or aria-label="Close event details"), keeping the existing onClick={ () => popModal() } and the XIcon intact.packages/sdks/sdk/src/index.ts (1)
98-112:⚠️ Potential issue | 🟠 MajorWrap
send()network dispatch intry/catchand avoid inline flush recursion.At Line 99, a rejected
this.api.fetch(...)currently bubbles out ofsend(). Also, triggeringflush()inline at Line 109 can recursively start more sends in the same call path. Please handle failures locally and schedule flush asynchronously after first session acquisition.Proposed fix
- // Disable keepalive for replay since it has a hard body limit and breaks the request - const result = await this.api.fetch< - TrackHandlerPayload, - { deviceId: string; sessionId: string } - >('/track', payload, { keepalive: payload.type !== 'replay' }); - this.deviceId = result?.deviceId; - const hadSession = !!this.sessionId; - this.sessionId = result?.sessionId; - - // Flush queued items (e.g. replay chunks) when sessionId first arrives - if (!hadSession && this.sessionId) { - this.flush(); - } - - return result; + try { + // Disable keepalive for replay since it has a hard body limit and breaks the request + const result = await this.api.fetch< + TrackHandlerPayload, + { deviceId: string; sessionId: string } + >('/track', payload, { keepalive: payload.type !== 'replay' }); + this.deviceId = result?.deviceId; + const hadSession = Boolean(this.sessionId); + this.sessionId = result?.sessionId; + + // Flush queued items (e.g. replay chunks) when sessionId first arrives + if (!hadSession && this.sessionId) { + queueMicrotask(() => { + void this.flush(); + }); + } + + return result; + } catch (error) { + this.log('Failed to send payload', error); + return undefined; + }As per coding guidelines: "Handle errors appropriately in async code with try-catch blocks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/sdk/src/index.ts` around lines 98 - 112, The network dispatch in send() must be wrapped in try/catch so a rejected this.api.fetch(...) does not bubble out; call this.api.fetch inside a try block, handle/log the error in the catch, and ensure send() returns a sensible value (e.g., undefined or an error result) instead of throwing. Also avoid invoking this.flush() synchronously inside the same call path after setting this.sessionId — schedule the flush asynchronously (e.g., via setTimeout(..., 0) or queueMicrotask) when !hadSession && this.sessionId so subsequent sends aren’t recursively re-entered; keep references to this.api.fetch, this.deviceId, this.sessionId, and flush() when applying the changes.apps/start/src/components/sessions/replay/index.tsx (1)
65-93:⚠️ Potential issue | 🟠 MajorFix stale-closure effect and unsafe recursive chunk loading.
At Line 65, the empty dependency array captures mutable values (
sessionId,projectId,fromIndex,queryClient,addEvent,refreshDuration,trpc), and Line 83 can recurse indefinitely whenhasMoreis true butres.datais empty. There’s also no cancellation guard for unmount/session changes.💡 Proposed fix
useEffect(() => { - function recursive(fromIndex: number) { - queryClient - .fetchQuery( - trpc.session.replayChunksFrom.queryOptions({ - sessionId, - projectId, - fromIndex, - }) - ) - .then((res) => { - res.data.forEach((row) => { - row?.events?.forEach((event) => { - addEvent(event); - }); - }); - refreshDuration(); - if (res.hasMore) { - recursive(fromIndex + res.data.length); - } - }) - .catch(() => { - // chunk loading failed — replay may be incomplete - }); - } - - recursive(fromIndex); - }, []); + let cancelled = false; + + const loadRemainingChunks = async () => { + let nextIndex = fromIndex; + try { + while (!cancelled) { + const res = await queryClient.fetchQuery( + trpc.session.replayChunksFrom.queryOptions({ + sessionId, + projectId, + fromIndex: nextIndex, + }), + ); + + for (const row of res.data) { + for (const event of row?.events ?? []) { + addEvent(event); + } + } + refreshDuration(); + + if (!res.hasMore || res.data.length === 0) break; + nextIndex += res.data.length; + } + } catch { + // chunk loading failed — replay may be incomplete + } + }; + + void loadRemainingChunks(); + return () => { + cancelled = true; + }; + }, [addEvent, fromIndex, projectId, queryClient, refreshDuration, sessionId, trpc]);Based on learnings, dependencies in hook dependency arrays should be specified correctly. As per coding guidelines, "Specify all dependencies in hook dependency arrays correctly" and "Use
async/awaitsyntax instead of promise chains for better readability."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/index.tsx` around lines 65 - 93, The useEffect with the inner recursive function captures stale values and can recurse forever when res.hasMore is true but res.data is empty, and it lacks cancellation on unmount or param changes; update useEffect to include all referenced dependencies (sessionId, projectId, fromIndex, queryClient, addEvent, refreshDuration, trpc), convert the fetchQuery promise chain to an async function using async/await, replace recursion with a safe loop that breaks if res.data.length === 0 to avoid infinite loops, and add an abort/cancel flag (or AbortController) checked before processing results so work stops when the component unmounts or session/project changes; keep the same identifiers (recursive function name or replace with a loop inside the effect, trpc.session.replayChunksFrom.queryOptions, addEvent, refreshDuration, queryClient.fetchQuery) so reviewers can locate and verify the fix.packages/db/src/buffers/replay-buffer.ts (1)
36-43:⚠️ Potential issue | 🟠 MajorValidate
UInt16-backed fields before enqueueing replay chunks.Line 36-43 accepts unbounded
chunk_index/events_count, but the schema defines both asUInt16(seepackages/db/code-migrations/10-add-session-replay.tsLine 21 and Line 24). Out-of-range values can repeatedly fail inserts and block healthy batch progress.🛠️ Suggested fix
+const MAX_UINT16 = 65_535 as const; + export class ReplayBuffer extends BaseBuffer { @@ async add(chunk: IClickhouseSessionReplayChunk) { try { + const isChunkIndexValid = + Number.isInteger(chunk.chunk_index) && + chunk.chunk_index >= 0 && + chunk.chunk_index <= MAX_UINT16; + const isEventsCountValid = + Number.isInteger(chunk.events_count) && + chunk.events_count >= 0 && + chunk.events_count <= MAX_UINT16; + + if (!isChunkIndexValid || !isEventsCountValid) { + throw new Error( + `Invalid replay chunk numeric bounds: chunk_index=${chunk.chunk_index}, events_count=${chunk.events_count}`, + ); + } + const redis = getRedisCache();#!/bin/bash set -euo pipefail echo "Schema constraints (should show UInt16 fields):" rg -n "chunk_index|events_count|UInt16" packages/db/code-migrations/10-add-session-replay.ts echo echo "ReplayBuffer numeric guards (should currently be missing):" rg -n "MAX_UINT16|65535|Number\\.isInteger\\(chunk\\.(chunk_index|events_count)\\)" packages/db/src/buffers/replay-buffer.ts || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/replay-buffer.ts` around lines 36 - 43, The add method is pushing IClickhouseSessionReplayChunk values without validating chunk_index and events_count against the UInt16 schema; add a guard at the start of replay-buffer.add that checks Number.isInteger(chunk.chunk_index) and Number.isInteger(chunk.events_count) and that both are within 0..65535 (use a MAX_UINT16 constant = 65535), and if validation fails do NOT call redis.multi()/rpush/incr/llen—instead log or throw a clear error and return so the invalid chunk isn't enqueued or counted.packages/validation/src/track.validation.ts (1)
66-73:⚠️ Potential issue | 🟠 MajorReplay time-window ordering validation is still missing.
Line 70 and Line 71 validate datetime format, but payloads with
ended_at < started_atstill pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/validation/src/track.validation.ts` around lines 66 - 73, zReplayPayload currently only checks datetime format but allows ended_at < started_at; add a refinement to zReplayPayload (using .refine or .superRefine on zReplayPayload) that parses/compares started_at and ended_at (convert the z.string().datetime() values to Date or use a preprocess/transform) and rejects the object if ended_at is earlier than started_at, returning a clear validation error referencing started_at/ended_at.packages/db/src/services/session.service.ts (3)
359-367:⚠️ Potential issue | 🟠 MajorReturn persisted replay chunk indexes.
chunkIndex: index + fromIndexcan drift from storedchunk_indexwhen rows are sparse/reordered.🛠️ Proposed fix
- .map((row, index) => { + .map((row) => { const events = getSafeJson< { type: number; data: unknown; timestamp: number }[] >(row.payload); if (!events) { return null; } - return { chunkIndex: index + fromIndex, events }; + return { chunkIndex: row.chunk_index, events }; })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/session.service.ts` around lines 359 - 367, The mapped chunkIndex is currently computed as index + fromIndex which can drift when DB rows are sparse/reordered; instead read the persisted chunk index from the row (e.g., use row.chunk_index or the actual column name holding the stored chunk index) and use that value when returning { chunkIndex, events }; ensure you parse/coerce it to a number (and still return null when events is falsy) so the returned chunkIndex always matches the stored DB value in the session.service.ts mapping block.
241-242:⚠️ Potential issue | 🟠 MajorAlign replay alias with transformer field.
transformSession()readssession.has_replay, but this select aliases tohasReplay. That can yieldhasReplay: undefinedin list responses.🛠️ Proposed fix
- sb.select.has_replay = `toBool(src.session_id != '') as hasReplay`; + sb.select.has_replay = `toBool(src.session_id != '') as has_replay`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/session.service.ts` around lines 241 - 242, The SELECT alias for replay is mismatched with transformSession: sb.select.has_replay currently sets `toBool(src.session_id != '') as hasReplay` but transformSession expects session.has_replay; update the alias to `has_replay` (i.e., `toBool(src.session_id != '') as has_replay`) in sb.select.has_replay so the query result maps correctly to transformSession, leaving the JOIN (sb.joins.has_replay) and session_replay_chunks subquery unchanged.
346-354: 🛠️ Refactor suggestion | 🟠 MajorUse the ClickHouse query builder for new replay queries.
The newly added replay queries are raw SQL strings. Please migrate these to the project ClickHouse query builder/query-functions stack.
As per coding guidelines
**/*.{ts,tsx,js,jsx}: “When writing ClickHouse queries, always use the custom query builder from./packages/db/src/clickhouse/query-builder.tsand./packages/db/src/clickhouse/query-functions.ts”.Also applies to: 385-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/session.service.ts` around lines 346 - 354, Replace the raw SQL string used in chQuery in session.service.ts with the project's ClickHouse query builder/query-functions: stop using string interpolation and sqlstring.escape for sessionId/projectId and instead import and use the builders from packages/db/src/clickhouse/query-builder.ts and query-functions.ts to construct the SELECT of chunk_index and payload from TABLE_NAMES.session_replay_chunks, add WHERE clauses for session_id and project_id using parameterized equality helpers, apply ORDER BY started_at, ended_at, chunk_index, and apply LIMIT (REPLAY_CHUNKS_PAGE_SIZE + 1) and OFFSET (fromIndex) via the builder before passing the generated query to chQuery; make the same change for the similar block at lines 385-391 to ensure all replay queries use the query-builder/query-functions stack.
🧹 Nitpick comments (13)
packages/common/src/id.ts (1)
7-8: Consider adding an explicit return type and validatinglengthedge cases.The implementation is correct and maintains backward compatibility. A few optional improvements:
- Per coding guidelines, adding an explicit return type enhances clarity.
- If
lengthis passed as0or a negative value,nanoidmay produce unexpected results (empty or invalid string).✨ Optional improvement with return type and length validation
-export function generateId(prefix?: string, length?: number) { - return prefix ? `${prefix}_${nanoid(length ?? 8)}` : nanoid(length ?? 8); +const DEFAULT_ID_LENGTH = 8; + +export function generateId(prefix?: string, length?: number): string { + const idLength = length && length > 0 ? length : DEFAULT_ID_LENGTH; + return prefix ? `${prefix}_${nanoid(idLength)}` : nanoid(idLength); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/src/id.ts` around lines 7 - 8, Add an explicit return type to generateId (e.g., string) and validate the length parameter before passing it to nanoid: ensure length is a positive integer (treat undefined as default, coerce or clamp values <= 0 to a sensible default like 1 or fall back to undefined) so nanoid never receives 0 or negative values; keep the current prefix logic using generateId and nanoid names to locate the function and update its signature and parameter handling accordingly.packages/sdks/web/src/replay/recorder.ts (3)
85-88: Consider removingconsole.errorbefore rethrowing.Per coding guidelines,
console.log,debugger, andalertshould be removed from production code. Logging then rethrowing causes double-logging if callers also handle errors. Either log and swallow (for resilience) or rethrow without logging (let caller decide).💡 Suggested alternatives
Option A – Silent rethrow (let caller log):
} catch (err) { - console.error('[ReplayRecorder] sendChunk failed', err); throw err; }Option B – Log and retain buffer for retry (no throw):
} catch (err) { - console.error('[ReplayRecorder] sendChunk failed', err); - throw err; + // Keep buffer intact for next flush attempt }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/replay/recorder.ts` around lines 85 - 88, In the catch block inside sendChunk in replay/recorder.ts, remove the direct console.error call so errors are not double-logged; simply rethrow the caught error (throw err) and let callers decide how to log/handle it (alternatively implement a retry/retain buffer strategy elsewhere if you need in-recorder resilience), ensuring the change is made in the catch handler of the sendChunk function.
66-68: Silently dropping oversized single events may complicate debugging.When a single event exceeds
maxPayloadBytes, it's dropped without any indication. Consider adding a warning to help diagnose issues with very large DOM snapshots.💡 Suggested fix
// Single event exceeds limit — drop it to avoid server rejection + console.warn('[ReplayRecorder] Dropping oversized event', buffer[0]?.type); buffer = []; return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/replay/recorder.ts` around lines 66 - 68, The code currently clears buffer and returns when a single event exceeds maxPayloadBytes, dropping it silently; update the branch that handles "Single event exceeds limit" so it logs a warning (include the event's byte size and the configured maxPayloadBytes) before clearing buffer and returning—refer to the buffer variable and maxPayloadBytes constant/parameter in recorder.ts and use the established logging mechanism (e.g., a logger instance or console.warn) so oversized DOM snapshots are visible in logs for debugging.
92-94: Consider extracting magic number2to a named constant.The comment clarifies that
2isEventType.FullSnapshot, but extracting it improves readability and reduces duplication across lines 59, 94, 117, 124, 131, and 142.💡 Suggested fix
+const FULL_SNAPSHOT_EVENT_TYPE = 2; // rrweb EventType.FullSnapshot + export function startReplayRecorder( ... - buffer.some((e) => e.type === 2); /* EventType.FullSnapshot */ + buffer.some((e) => e.type === FULL_SNAPSHOT_EVENT_TYPE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/replay/recorder.ts` around lines 92 - 94, Replace the magic literal 2 with a named constant by importing or declaring the EventType enum (or a constant) and using EventType.FullSnapshot wherever the code currently uses 2 (e.g., the isFullSnapshot calculation that uses buffer.some((e) => e.type === 2)). Update all other occurrences mentioned in the review (the spots around the checks at the same conceptual places) to reference EventType.FullSnapshot so the intent is explicit and duplicates are removed; ensure the import or declaration is added near the top of recorder.ts and all comparisons use the named symbol instead of the numeric literal.packages/sdks/web/src/types.debug.ts (1)
33-33: Missing test forgetSessionId.The type definitions in
types.d.tsadded bothgetDeviceIdandgetSessionIdtoExposedMethodsNames, but onlygetDeviceIdis tested here. Consider adding a test call forgetSessionIdto maintain consistency and verify the type works correctly.♻️ Proposed fix
window.op.fetchDeviceId(); window.op.getDeviceId(); + window.op.getSessionId();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/types.debug.ts` at line 33, Add a test invocation for getSessionId alongside the existing getDeviceId call to verify the ExposedMethodsNames type; update the debug/types test file (where window.op.getDeviceId() is called) to also call window.op.getSessionId() so both newly added methods are exercised by the type-check.apps/start/src/components/sessions/replay/replay-context.tsx (1)
179-198: Consider memoizing the context value object.The
valueobject is recreated on every render. While most properties are stable callbacks (viauseCallback), the object identity itself changes, which could cause unnecessary re-renders in deeply nested consumers that depend on the entire context.♻️ Proposed refactor using useMemo
+ const value: ReplayContextValue = useMemo( + () => ({ + currentTimeRef, + subscribeToCurrentTime, + isPlaying, + duration, + startTime, + isReady, + play, + pause, + toggle, + seek, + setSpeed, + addEvent, + refreshDuration, + onPlayerReady, + onPlayerDestroy, + setCurrentTime, + setIsPlaying: setIsPlayingWithRef, + setDuration, + }), + [ + isPlaying, + duration, + startTime, + isReady, + play, + pause, + toggle, + seek, + setSpeed, + addEvent, + refreshDuration, + onPlayerReady, + onPlayerDestroy, + setCurrentTime, + setIsPlayingWithRef, + setDuration, + subscribeToCurrentTime, + ], + ); - const value: ReplayContextValue = { - currentTimeRef, - subscribeToCurrentTime, - isPlaying, - duration, - startTime, - isReady, - play, - pause, - toggle, - seek, - setSpeed, - addEvent, - refreshDuration, - onPlayerReady, - onPlayerDestroy, - setCurrentTime, - setIsPlaying: setIsPlayingWithRef, - setDuration, - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-context.tsx` around lines 179 - 198, The context value object named value (of type ReplayContextValue) is recreated on every render which can trigger unnecessary re-renders in consumers; wrap the construction of value in a useMemo so its identity only changes when any of its dependencies change (include currentTimeRef, subscribeToCurrentTime, isPlaying, duration, startTime, isReady, play, pause, toggle, seek, setSpeed, addEvent, refreshDuration, onPlayerReady, onPlayerDestroy, setCurrentTime, setIsPlayingWithRef, setDuration) to stabilize the object identity and avoid extra renders.apps/start/src/components/sessions/replay/browser-chrome.tsx (1)
17-17: Clarify theurlprop type for the "hide URL bar" behavior.The
url !== falsecheck on line 32 impliesfalsehas special semantics (hide the URL bar), but this isn't obvious from theReactNodetype. Consider using a more explicit union type.♻️ Proposed type clarification
}: { - url?: ReactNode; + url?: ReactNode | false; // Pass `false` to hide the URL bar entirely children: ReactNode;Note:
ReactNodetechnically includesfalse, so this is purely for documentation clarity.Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/browser-chrome.tsx` at line 17, The prop declaration for url should explicitly include false to document the "hide URL bar" semantics: change the type from url?: ReactNode to url?: ReactNode | false in the BrowserChrome component's props (and any related prop/interface/type aliases), keep the existing runtime check (url !== false) unchanged, and update any JSDoc or prop comments for BrowserChrome to state that false hides the URL bar.apps/start/src/components/sessions/replay/replay-event-feed.tsx (1)
86-101: Consider extracting skeleton count as a constant.Minor readability improvement — the magic number
5could be a named constant.♻️ Proposed refactor
+const SKELETON_COUNT = 5; + export function ReplayEventFeed({ events, replayLoading }: { events: IServiceEvent[]; replayLoading: boolean }) {{replayLoading && - Array.from({ length: 5 }).map((_, i) => ( + Array.from({ length: SKELETON_COUNT }).map((_, i) => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/sessions/replay/replay-event-feed.tsx` around lines 86 - 101, The skeleton list uses a magic number 5; extract that into a named constant (e.g., SKELETON_COUNT) and replace Array.from({ length: 5 }) with Array.from({ length: SKELETON_COUNT }) to improve readability; add the constant near the top of the component (or module) so replayLoading rendering code (the block using Array.from, key={i}, and the animated skeleton divs) references SKELETON_COUNT instead of the literal 5.packages/db/src/buffers/session-buffer.ts (1)
188-195: Extract the Redis session TTL into a named constant.Line 188 and Line 195 repeat
60 * 60. Please lift this into a descriptive constant to avoid magic numbers and keep TTL changes single-source.♻️ Suggested refactor
export class SessionBuffer extends BaseBuffer { + private static readonly SESSION_CACHE_TTL_SECONDS = 60 * 60 as const; @@ multi.set( `session:${newSession.id}`, JSON.stringify(newSession), 'EX', - 60 * 60 + SessionBuffer.SESSION_CACHE_TTL_SECONDS ); @@ multi.set( `session:${newSession.project_id}:${newSession.profile_id}`, JSON.stringify(newSession), 'EX', - 60 * 60 + SessionBuffer.SESSION_CACHE_TTL_SECONDS );As per coding guidelines,
**/*.{js,ts,jsx,tsx}: "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/session-buffer.ts` around lines 188 - 195, The code repeats the literal TTL value 60 * 60 when setting Redis keys (seen around the use of multi.set with newSession and keys like `session:${newSession.project_id}:${newSession.profile_id}`); extract this into a descriptive constant (e.g., SESSION_TTL_SECONDS = 60 * 60) at the top of the module and replace both occurrences with that constant so TTL changes are single-source and remove the magic numbers.packages/db/src/buffers/index.ts (1)
13-16: Avoid adding new re-exports to the barrelindex.ts.Line 13 and Line 16 expand a barrel file. Prefer importing
ReplayBuffer/IClickhouseSessionReplayChunkdirectly from./replay-bufferat usage sites to keep module boundaries explicit.As per coding guidelines,
**/index.{js,ts,jsx,tsx}: "Avoid barrel files (index files that re-export everything)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/index.ts` around lines 13 - 16, The barrel index.ts currently re-exports a ReplayBuffer instance (replayBuffer) and types (IClickhouseSessionReplayChunk), which violates the guideline to avoid expanding barrel files; remove the exported const and type re-exports from this file and instead import ReplayBuffer/IClickhouseSessionReplayChunk (and ProfileBackfillEntry) directly from their source modules (./replay-buffer and ./profile-backfill-buffer) at every usage site; update all files that currently import from this barrel to reference the original modules and delete the two export lines (export const replayBuffer = new ReplayBuffer(); and the export type {...} lines) from packages/db/src/buffers/index.ts.apps/start/src/routes/_app.$organizationId.$projectId.sessions_.$sessionId.tsx (1)
327-332: Potential type mismatch inpathsmapping.The filter ensures
e.pathis truthy, but TypeScript may still infere.pathasstring | undefined. Consider using a type guard or non-null assertion after the filter.💡 Suggested fix
<VisitedRoutes paths={events .filter((e) => e.name === 'screen_view' && e.path) - .map((e) => e.path)} + .map((e) => e.path!)} />Or use a type-narrowing filter:
<VisitedRoutes - paths={events - .filter((e) => e.name === 'screen_view' && e.path) - .map((e) => e.path)} + paths={events + .filter((e): e is typeof e & { path: string } => e.name === 'screen_view' && !!e.path) + .map((e) => e.path)} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/routes/_app`.$organizationId.$projectId.sessions_.$sessionId.tsx around lines 327 - 332, VisitedRoutes is receiving paths mapped from events where e.path may still be inferred as string | undefined; narrow the type before mapping by adding a type guard or non-null assertion so map yields string[] only. For example, change the filter on events (used to build paths for VisitedRoutes) to a type-narrowing predicate like filter((e): e is { path: string } => !!e.path) or keep the existing filter and use .map(e => e.path!) to assert non-null, ensuring VisitedRoutes receives string[] instead of (string | undefined)[].apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.sessions.tsx (1)
23-23: Consider naming the page size constant.
take: 50would be easier to maintain as a named constant (e.g.,SESSIONS_PAGE_SIZE).As per coding guidelines "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/routes/_app`.$organizationId.$projectId.profiles.$profileId._tabs.sessions.tsx at line 23, Replace the magic number 50 with a named constant: declare a descriptive constant (e.g., SESSIONS_PAGE_SIZE = 50) near the top of this module (or exported from a constants file) and use that constant in the query/options object where you currently have take: 50; update any other occurrences in this file that use the same page size to reference SESSIONS_PAGE_SIZE to improve clarity and maintainability (check usages around the sessions loader/route handlers that construct the pagination params).packages/validation/src/track.validation.ts (1)
67-72: Extract replay limits into named constants for readability.Inline limits are hard to reason about during maintenance; named constants would make intent clearer.
As per coding guidelines "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/validation/src/track.validation.ts` around lines 67 - 72, Extract the magic numeric limits into clearly named constants (e.g., MAX_CHUNK_INDEX = 65_535, MIN_EVENTS_COUNT = 1, MAX_PAYLOAD_BYTES = 1_048_576 * 2) and replace inline literals in the Zod schema (fields chunk_index, events_count, payload, and any min/max uses) with those constants; declare the constants near the top of packages/validation/src/track.validation.ts (or immediately above the schema) so the schema reads z.number().int().min(0).max(MAX_CHUNK_INDEX), z.number().int().min(MIN_EVENTS_COUNT), and z.string().max(MAX_PAYLOAD_BYTES) for improved readability and maintainability.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (66)
.claude/CLAUDE.mdapps/api/src/controllers/event.controller.tsapps/api/src/controllers/track.controller.tsapps/api/src/hooks/duplicate.hook.tsapps/api/src/hooks/request-logging.hook.tsapps/api/src/index.tsapps/api/src/routes/track.router.tsapps/api/src/utils/ids.tsapps/public/content/docs/(tracking)/revenue-tracking.mdxapps/public/content/docs/(tracking)/sdks/astro.mdxapps/public/content/docs/(tracking)/sdks/nextjs.mdxapps/public/content/guides/ecommerce-tracking.mdxapps/public/content/guides/nextjs-analytics.mdxapps/public/public/op1-replay.jsapps/public/public/op1.jsapps/public/src/app/layout.tsxapps/start/package.jsonapps/start/src/components/events/table/columns.tsxapps/start/src/components/sessions/replay/browser-chrome.tsxapps/start/src/components/sessions/replay/index.tsxapps/start/src/components/sessions/replay/replay-context.tsxapps/start/src/components/sessions/replay/replay-controls.tsxapps/start/src/components/sessions/replay/replay-event-feed.tsxapps/start/src/components/sessions/replay/replay-event-item.tsxapps/start/src/components/sessions/replay/replay-player.tsxapps/start/src/components/sessions/replay/replay-timeline.tsxapps/start/src/components/sessions/replay/replay-utils.tsapps/start/src/components/sessions/table/columns.tsxapps/start/src/components/ui/scroll-area.tsxapps/start/src/hooks/use-page-tabs.tsapps/start/src/modals/event-details.tsxapps/start/src/routeTree.gen.tsapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.sessions.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.tsxapps/start/src/routes/_app.$organizationId.$projectId.sessions_.$sessionId.tsxapps/start/src/types/rrweb-player.d.tsapps/start/src/utils/op.tsapps/worker/src/boot-cron.tsapps/worker/src/jobs/cron.tsapps/worker/src/jobs/events.incoming-event.tsapps/worker/src/jobs/events.incoming-events.test.tsapps/worker/src/metrics.tsapps/worker/src/utils/session-handler.tspackages/common/src/id.tspackages/db/code-migrations/10-add-session-replay.tspackages/db/code-migrations/migrate.tspackages/db/src/buffers/index.tspackages/db/src/buffers/replay-buffer.tspackages/db/src/buffers/session-buffer.tspackages/db/src/clickhouse/client.tspackages/db/src/services/session.service.tspackages/queue/src/queues.tspackages/sdks/astro/src/OpenPanelComponent.astropackages/sdks/nextjs/index.tsxpackages/sdks/sdk/src/index.tspackages/sdks/web/package.jsonpackages/sdks/web/src/index.tspackages/sdks/web/src/replay/index.tspackages/sdks/web/src/replay/recorder.tspackages/sdks/web/src/types.d.tspackages/sdks/web/src/types.debug.tspackages/sdks/web/tsup.config.tspackages/trpc/src/routers/session.tspackages/validation/src/track.validation.tstest.tstooling/publish/publish.ts
💤 Files with no reviewable changes (1)
- test.ts
Comment on lines
+218
to
222
| deviceId, | ||
| sessionId, | ||
| currentDeviceId: '', // TODO: Remove | ||
| previousDeviceId: '', // TODO: Remove | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nP --type=ts -C3 '\b(currentDeviceId|previousDeviceId)\b'Repository: Openpanel-dev/openpanel
Length of output: 18504
Do not send empty placeholders for device fields still consumed by session handler logic.
currentDeviceId and previousDeviceId are actively used in downstream session stitching (apps/worker/src/utils/session-handler.ts line 138-154). The code checks these fields to lookup existing session jobs via Redis. Sending empty strings will cause this conditional to fail silently, bypassing session stitching entirely.
Note: These values are already being computed in this controller (lines 411-423 in track.controller.ts); passing them through would maintain the correct behavior. If removing these fields, migrate the session handler to use deviceId first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/controllers/track.controller.ts` around lines 218 - 222, The
payload is sending empty placeholders for currentDeviceId and previousDeviceId
which breaks downstream session stitching (see session-handler.ts logic); update
the object built in track.controller.ts to pass the computed currentDeviceId and
previousDeviceId values already calculated in this controller (instead of ''),
ensuring the properties match the names used by the session handler
(currentDeviceId, previousDeviceId) so Redis lookups and session stitching
continue to work; do not remove these fields here unless you also migrate
session-handler.ts to fall back to deviceId first.
Comment on lines
+85
to
+87
| } catch (error) { | ||
| console.error('Error getting session end GET /track/device-id', error); | ||
| } |
Contributor
There was a problem hiding this comment.
Replace console.error with a proper logger.
Per coding guidelines, console.log, debugger, and alert statements should be removed from production code. Consider using a structured logger for error reporting.
💡 Suggested fix
+import { logger } from '../utils/logger';
// ...
} catch (error) {
- console.error('Error getting session end GET /track/device-id', error);
+ logger.error('Error getting session end', { error, path: 'GET /track/device-id' });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/utils/ids.ts` around lines 85 - 87, In apps/api/src/utils/ids.ts
replace the console.error in the catch block (the error handling for the GET
/track/device-id / session end flow) with your structured logger: import and use
the project's logger (e.g., logger.error) inside the catch for the function
handling session end (referenced in this file) and pass a descriptive message
plus the caught error object/stack and any relevant context (request id/device
id) instead of console.error; remove any leftover console.* usage.
| @@ -1 +1 @@ | |||
| "use strict";(()=>{var u=class{constructor(e){this.baseUrl=e.baseUrl,this.headers={"Content-Type":"application/json",...e.defaultHeaders},this.maxRetries=e.maxRetries??3,this.initialRetryDelay=e.initialRetryDelay??500}async resolveHeaders(){let e={};for(let[i,t]of Object.entries(this.headers)){let s=await t;s!==null&&(e[i]=s)}return e}addHeader(e,i){this.headers[e]=i}async post(e,i,t,s){try{let n=await fetch(e,{method:"POST",headers:await this.resolveHeaders(),body:i?JSON.stringify(i??{}):void 0,keepalive:!0,...t});if(n.status===401)return null;if(n.status!==200&&n.status!==202)throw new Error(`HTTP error! status: ${n.status}`);let r=await n.text();return r?JSON.parse(r):null}catch(n){if(s<this.maxRetries){let r=this.initialRetryDelay*2**s;return await new Promise(a=>setTimeout(a,r)),this.post(e,i,t,s+1)}return console.error("Max retries reached:",n),null}}async fetch(e,i,t={}){let s=`${this.baseUrl}${e}`;return this.post(s,i,t,0)}},l=class{constructor(e){this.options=e,this.queue=[];let i={"openpanel-client-id":e.clientId};e.clientSecret&&(i["openpanel-client-secret"]=e.clientSecret),i["openpanel-sdk-name"]=e.sdk||"node",i["openpanel-sdk-version"]=e.sdkVersion||"1.0.3",this.api=new u({baseUrl:e.apiUrl||"https://api.openpanel.dev",defaultHeaders:i})}init(){}ready(){this.options.waitForProfile=!1,this.flush()}async send(e){return this.options.disabled||this.options.filter&&!this.options.filter(e)?Promise.resolve():this.options.waitForProfile&&!this.profileId?(this.queue.push(e),Promise.resolve()):this.api.fetch("/track",e)}setGlobalProperties(e){this.global={...this.global,...e}}async track(e,i){return this.log("track event",e,i),this.send({type:"track",payload:{name:e,profileId:i?.profileId??this.profileId,properties:{...this.global??{},...i??{}}}})}async identify(e){if(this.log("identify user",e),e.profileId&&(this.profileId=e.profileId,this.flush()),Object.keys(e).length>1)return this.send({type:"identify",payload:{...e,properties:{...this.global,...e.properties}}})}async alias(e){}async increment(e){return this.send({type:"increment",payload:e})}async decrement(e){return this.send({type:"decrement",payload:e})}async revenue(e,i){let t=i?.deviceId;return delete i?.deviceId,this.track("revenue",{...i??{},...t?{__deviceId:t}:{},__revenue:e})}async fetchDeviceId(){return(await this.api.fetch("/track/device-id",void 0,{method:"GET",keepalive:!1}))?.deviceId??""}clear(){this.profileId=void 0}flush(){this.queue.forEach(e=>{this.send({...e,payload:{...e.payload,profileId:e.payload.profileId??this.profileId}})}),this.queue=[]}log(...e){this.options.debug&&console.log("[OpenPanel.dev]",...e)}};function h(e){return e.replace(/([-_][a-z])/gi,i=>i.toUpperCase().replace("-","").replace("_",""))}var p=class extends l{constructor(t){super({sdk:"web",sdkVersion:"1.0.6",...t});this.options=t;this.lastPath="";this.pendingRevenues=[];if(!this.isServer()){try{let s=sessionStorage.getItem("openpanel-pending-revenues");if(s){let n=JSON.parse(s);Array.isArray(n)&&(this.pendingRevenues=n)}}catch{this.pendingRevenues=[]}this.setGlobalProperties({__referrer:document.referrer}),this.options.trackScreenViews&&(this.trackScreenViews(),setTimeout(()=>this.screenView(),0)),this.options.trackOutgoingLinks&&this.trackOutgoingLinks(),this.options.trackAttributes&&this.trackAttributes()}}debounce(t,s){clearTimeout(this.debounceTimer),this.debounceTimer=setTimeout(t,s)}isServer(){return typeof document>"u"}trackOutgoingLinks(){this.isServer()||document.addEventListener("click",t=>{let s=t.target,n=s.closest("a");if(n&&s){let r=n.getAttribute("href");if(r?.startsWith("http"))try{let a=new URL(r),o=window.location.hostname;a.hostname!==o&&super.track("link_out",{href:r,text:n.innerText||n.getAttribute("title")||s.getAttribute("alt")||s.getAttribute("title")})}catch{}}})}trackScreenViews(){if(this.isServer())return;let t=history.pushState;history.pushState=function(...a){let o=t.apply(this,a);return window.dispatchEvent(new Event("pushstate")),window.dispatchEvent(new Event("locationchange")),o};let s=history.replaceState;history.replaceState=function(...a){let o=s.apply(this,a);return window.dispatchEvent(new Event("replacestate")),window.dispatchEvent(new Event("locationchange")),o},window.addEventListener("popstate",()=>{window.dispatchEvent(new Event("locationchange"))});let n=()=>this.debounce(()=>this.screenView(),50);this.options.trackHashChanges?window.addEventListener("hashchange",n):window.addEventListener("locationchange",n)}trackAttributes(){this.isServer()||document.addEventListener("click",t=>{let s=t.target,n=s.closest("button"),r=s.closest("a"),a=n?.getAttribute("data-track")?n:r?.getAttribute("data-track")?r:null;if(a){let o={};for(let c of a.attributes)c.name.startsWith("data-")&&c.name!=="data-track"&&(o[h(c.name.replace(/^data-/,""))]=c.value);let d=a.getAttribute("data-track");d&&super.track(d,o)}})}screenView(t,s){if(this.isServer())return;let n,r;typeof t=="string"?(n=t,r=s):(n=window.location.href,r=t),this.lastPath!==n&&(this.lastPath=n,super.track("screen_view",{...r??{},__path:n,__title:document.title}))}async flushRevenue(){let t=this.pendingRevenues.map(s=>super.revenue(s.amount,s.properties));await Promise.all(t),this.clearRevenue()}clearRevenue(){if(this.pendingRevenues=[],!this.isServer())try{sessionStorage.removeItem("openpanel-pending-revenues")}catch{}}pendingRevenue(t,s){if(this.pendingRevenues.push({amount:t,properties:s}),!this.isServer())try{sessionStorage.setItem("openpanel-pending-revenues",JSON.stringify(this.pendingRevenues))}catch{}}};(e=>{if(e.op){let i=e.op.q||[],t=new p(i.shift()[1]);i.forEach(n=>{n[0]in t&&t[n[0]](...n.slice(1))});let s=new Proxy((n,...r)=>{let a=t[n]?t[n].bind(t):void 0;typeof a=="function"?a(...r):console.warn(`OpenPanel: ${n} is not a function`)},{get(n,r){if(r==="q")return;let a=t[r];return typeof a=="function"?a.bind(t):a}});e.op=s,e.openpanel=t}})(window);})(); | |||
| "use strict";(()=>{var g=Object.create;var h=Object.defineProperty;var m=Object.getOwnPropertyDescriptor;var v=Object.getOwnPropertyNames;var P=Object.getPrototypeOf,k=Object.prototype.hasOwnProperty;var b=(a,t)=>()=>(t||a((t={exports:{}}).exports,t),t.exports);var R=(a,t,e,s)=>{if(t&&typeof t=="object"||typeof t=="function")for(let n of v(t))!k.call(a,n)&&n!==e&&h(a,n,{get:()=>t[n],enumerable:!(s=m(t,n))||s.enumerable});return a};var w=(a,t,e)=>(e=a!=null?g(P(a)):{},R(t||!a||!a.__esModule?h(e,"default",{value:a,enumerable:!0}):e,a));var f=b((A,y)=>{"use strict";y.exports={}});var c=class{constructor(t){this.baseUrl=t.baseUrl,this.headers={"Content-Type":"application/json",...t.defaultHeaders},this.maxRetries=t.maxRetries??3,this.initialRetryDelay=t.initialRetryDelay??500}async resolveHeaders(){let t={};for(let[e,s]of Object.entries(this.headers)){let n=await s;n!==null&&(t[e]=n)}return t}addHeader(t,e){this.headers[t]=e}async post(t,e,s,n){try{let r=await fetch(t,{method:"POST",headers:await this.resolveHeaders(),body:e?JSON.stringify(e??{}):void 0,keepalive:!0,...s});if(r.status===401)return null;if(r.status!==200&&r.status!==202)throw new Error(`HTTP error! status: ${r.status}`);let i=await r.text();return i?JSON.parse(i):null}catch(r){if(n<this.maxRetries){let i=this.initialRetryDelay*2**n;return await new Promise(o=>setTimeout(o,i)),this.post(t,e,s,n+1)}return console.error("Max retries reached:",r),null}}async fetch(t,e,s={}){let n=`${this.baseUrl}${t}`;return this.post(n,e,s,0)}};var l=class{constructor(t){this.options=t;this.queue=[];let e={"openpanel-client-id":t.clientId};t.clientSecret&&(e["openpanel-client-secret"]=t.clientSecret),e["openpanel-sdk-name"]=t.sdk||"node",e["openpanel-sdk-version"]=t.sdkVersion||process.env.SDK_VERSION,this.api=new c({baseUrl:t.apiUrl||"https://api.openpanel.dev",defaultHeaders:e})}init(){}ready(){this.options.waitForProfile=!1,this.flush()}shouldQueue(t){return!!(t.type==="replay"&&!this.sessionId||this.options.waitForProfile&&!this.profileId)}async send(t){if(this.options.disabled||this.options.filter&&!this.options.filter(t))return Promise.resolve();if(this.shouldQueue(t))return this.queue.push(t),Promise.resolve();let e=await this.api.fetch("/track",t);this.deviceId=e?.deviceId;let s=!!this.sessionId;return this.sessionId=e?.sessionId,!s&&this.sessionId&&this.flush(),e}setGlobalProperties(t){this.global={...this.global,...t}}async track(t,e){return this.log("track event",t,e),this.send({type:"track",payload:{name:t,profileId:e?.profileId??this.profileId,properties:{...this.global??{},...e??{}}}})}async identify(t){if(this.log("identify user",t),t.profileId&&(this.profileId=t.profileId,this.flush()),Object.keys(t).length>1)return this.send({type:"identify",payload:{...t,properties:{...this.global,...t.properties}}})}async alias(t){}async increment(t){return this.send({type:"increment",payload:t})}async decrement(t){return this.send({type:"decrement",payload:t})}async revenue(t,e){let s=e?.deviceId;return delete e?.deviceId,this.track("revenue",{...e??{},...s?{__deviceId:s}:{},__revenue:t})}getDeviceId(){return this.deviceId??""}getSessionId(){return this.sessionId??""}async fetchDeviceId(){return Promise.resolve(this.deviceId??"")}clear(){this.profileId=void 0,this.deviceId=void 0,this.sessionId=void 0}flush(){let t=[];for(let e of this.queue){if(this.shouldQueue(e)){t.push(e);continue}let s=e.type==="replay"?e.payload:{...e.payload,profileId:"profileId"in e.payload?e.payload.profileId??this.profileId:this.profileId};this.send({...e,payload:s})}this.queue=t}log(...t){this.options.debug&&console.log("[OpenPanel.dev]",...t)}};function I(a){return a.replace(/([-_][a-z])/gi,t=>t.toUpperCase().replace("-","").replace("_",""))}var p=class extends l{constructor(e){super({sdk:"web",sdkVersion:"1.0.9",...e});this.options=e;this.lastPath="";this.pendingRevenues=[];if(!this.isServer()){try{let s=sessionStorage.getItem("openpanel-pending-revenues");if(s){let n=JSON.parse(s);Array.isArray(n)&&(this.pendingRevenues=n)}}catch{this.pendingRevenues=[]}if(this.setGlobalProperties({__referrer:document.referrer}),this.options.trackScreenViews&&(this.trackScreenViews(),setTimeout(()=>this.screenView(),0)),this.options.trackOutgoingLinks&&this.trackOutgoingLinks(),this.options.trackAttributes&&this.trackAttributes(),this.options.sessionReplay?.enabled){let s=this.options.sessionReplay.sampleRate??1;Math.random()<s&&this.loadReplayModule().then(r=>{r&&r.startReplayRecorder(this.options.sessionReplay,i=>{this.send({type:"replay",payload:{...i}})})})}}}async loadReplayModule(){try{{let e=this.options.sessionReplay?.scriptUrl??"https://openpanel.dev/op1-replay.js";return window.__openpanel_replay?window.__openpanel_replay:new Promise(s=>{let n=document.createElement("script");n.src=e,n.onload=()=>{s(window.__openpanel_replay??null)},n.onerror=()=>{console.warn("[OpenPanel] Failed to load replay script from",e),s(null)},document.head.appendChild(n)})}return await Promise.resolve().then(()=>w(f(),1))}catch(e){return console.warn("[OpenPanel] Failed to load replay module",e),null}}debounce(e,s){clearTimeout(this.debounceTimer),this.debounceTimer=setTimeout(e,s)}isServer(){return typeof document>"u"}trackOutgoingLinks(){this.isServer()||document.addEventListener("click",e=>{let s=e.target,n=s.closest("a");if(n&&s){let r=n.getAttribute("href");if(r?.startsWith("http"))try{let i=new URL(r),o=window.location.hostname;i.hostname!==o&&super.track("link_out",{href:r,text:n.innerText||n.getAttribute("title")||s.getAttribute("alt")||s.getAttribute("title")})}catch{}}})}trackScreenViews(){if(this.isServer())return;let e=history.pushState;history.pushState=function(...i){let o=e.apply(this,i);return window.dispatchEvent(new Event("pushstate")),window.dispatchEvent(new Event("locationchange")),o};let s=history.replaceState;history.replaceState=function(...i){let o=s.apply(this,i);return window.dispatchEvent(new Event("replacestate")),window.dispatchEvent(new Event("locationchange")),o},window.addEventListener("popstate",()=>{window.dispatchEvent(new Event("locationchange"))});let n=()=>this.debounce(()=>this.screenView(),50);this.options.trackHashChanges?window.addEventListener("hashchange",n):window.addEventListener("locationchange",n)}trackAttributes(){this.isServer()||document.addEventListener("click",e=>{let s=e.target,n=s.closest("button"),r=s.closest("a"),i=n?.getAttribute("data-track")?n:r?.getAttribute("data-track")?r:null;if(i){let o={};for(let d of i.attributes)d.name.startsWith("data-")&&d.name!=="data-track"&&(o[I(d.name.replace(/^data-/,""))]=d.value);let u=i.getAttribute("data-track");u&&super.track(u,o)}})}screenView(e,s){if(this.isServer())return;let n,r;typeof e=="string"?(n=e,r=s):(n=window.location.href,r=e),this.lastPath!==n&&(this.lastPath=n,super.track("screen_view",{...r??{},__path:n,__title:document.title}))}async flushRevenue(){let e=this.pendingRevenues.map(s=>super.revenue(s.amount,s.properties));await Promise.all(e),this.clearRevenue()}clearRevenue(){if(this.pendingRevenues=[],!this.isServer())try{sessionStorage.removeItem("openpanel-pending-revenues")}catch{}}pendingRevenue(e,s){if(this.pendingRevenues.push({amount:e,properties:s}),!this.isServer())try{sessionStorage.setItem("openpanel-pending-revenues",JSON.stringify(this.pendingRevenues))}catch{}}};(a=>{if(a.op){let t=a.op.q||[],e=new p(t.shift()[1]);t.forEach(n=>{n[0]in e&&e[n[0]](...n.slice(1))});let s=new Proxy((n,...r)=>{let i=e[n]?e[n].bind(e):void 0;typeof i=="function"?i(...r):console.warn(`OpenPanel: ${n} is not a function`)},{get(n,r){if(r==="q")return;let i=e[r];return typeof i=="function"?i.bind(e):i}});a.op=s,a.openpanel=e}})(window);})(); | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the source file that generates this bundle
fd -t f "op1" --extension js --extension ts --extension jsx --extension tsxRepository: Openpanel-dev/openpanel
Length of output: 125
🏁 Script executed:
# Search for SDK_VERSION and process.env references in the codebase
rg -n "SDK_VERSION|process\.env" --type ts --type js -C 3Repository: Openpanel-dev/openpanel
Length of output: 50380
🏁 Script executed:
# Check the directory structure to understand the bundle location
find apps/public -type f -name "*.ts" -o -name "*.js" | head -20Repository: Openpanel-dev/openpanel
Length of output: 769
🏁 Script executed:
# Look for build configuration files
fd -t f "package.json|tsconfig|webpack|esbuild|vite" apps/publicRepository: Openpanel-dev/openpanel
Length of output: 222
🏁 Script executed:
# Find the build configuration that generates op1.js
rg -n "op1" apps/public package.json vite.config esbuild --type json --type tsRepository: Openpanel-dev/openpanel
Length of output: 166
🏁 Script executed:
# Check packages/sdks/web source code structure
fd -t f . packages/sdks/web/src | head -20Repository: Openpanel-dev/openpanel
Length of output: 317
🏁 Script executed:
# Look at the actual source file that likely generates this bundle
cat packages/sdks/web/src/index.ts | head -100Repository: Openpanel-dev/openpanel
Length of output: 2852
🏁 Script executed:
# Check for build/bundle configuration
find apps/public -name "*.config.*" -type fRepository: Openpanel-dev/openpanel
Length of output: 186
🏁 Script executed:
# Check the main package.json for build scripts that might build op1.js
cat packages/sdks/web/package.json 2>/dev/null || echo "not found"Repository: Openpanel-dev/openpanel
Length of output: 614
🏁 Script executed:
# Look at source.config.ts which might define the bundle
cat apps/public/source.config.tsRepository: Openpanel-dev/openpanel
Length of output: 2076
🏁 Script executed:
# Check if there's a dedicated build script for op1.js
rg -n "op1" --type ts --type js --type json -C 2Repository: Openpanel-dev/openpanel
Length of output: 14035
🏁 Script executed:
# Look for esbuild or bundle configs in packages/sdks/web
find packages/sdks/web -name "*.config.*" -o -name "tsconfig.json" | xargs cat 2>/dev/nullRepository: Openpanel-dev/openpanel
Length of output: 2645
🏁 Script executed:
# Check the tracker.ts source file that's used for the IIFE build
cat packages/sdks/web/src/tracker.tsRepository: Openpanel-dev/openpanel
Length of output: 1473
🏁 Script executed:
# Check if WEB_VERSION is defined at build time
rg -n "WEB_VERSION|SDK_VERSION" packages/sdks --type ts -B 2 -A 2Repository: Openpanel-dev/openpanel
Length of output: 701
🏁 Script executed:
# Verify the actual tsup build output
find packages/sdks/web/dist -name "*.js" 2>/dev/null | head -5Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
# Check if there's an env substitution in the build process
rg -n "WEB_VERSION|SDK_VERSION|define" tooling --type ts --type jsonRepository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
# Check if the base SDK class is bundled into op1.js by searching for the relevant code
grep -o "openpanel-sdk-version" packages/sdks/web/dist/src/tracker.global.js 2>/dev/null | head -1 || echo "Not found or dist doesn't exist"Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
# Let's check the actual content of dist files to understand what's bundled
ls -la packages/sdks/web/dist/src/ 2>/dev/null || echo "dist/ directory doesn't exist"Repository: Openpanel-dev/openpanel
Length of output: 96
🏁 Script executed:
# Look at the base SDK constructor to see the full fallback chain
cat packages/sdks/sdk/src/index.ts | head -100Repository: Openpanel-dev/openpanel
Length of output: 2462
🏁 Script executed:
# Check how the web SDK passes sdkVersion to the base class
cat packages/sdks/web/src/index.ts | grep -A 10 "constructor"Repository: Openpanel-dev/openpanel
Length of output: 358
🏁 Script executed:
# Check if WEB_VERSION is injected at build time via define/env vars
rg -n "WEB_VERSION" tooling packages --type ts --type json -B 2 -A 2Repository: Openpanel-dev/openpanel
Length of output: 336
🏁 Script executed:
# Check pnpm config or build scripts
cat pnpm-workspace.yaml 2>/dev/null || cat .npmrc 2>/dev/null || echo "No pnpm config found"Repository: Openpanel-dev/openpanel
Length of output: 364
🏁 Script executed:
# Check if there's a script that runs before/after build
cat packages/sdks/web/package.json | grep -A 10 "scripts"Repository: Openpanel-dev/openpanel
Length of output: 359
🏁 Script executed:
# Search for version references in the repository
rg -n "1\.0\." packages/sdks/web --type ts --type json | head -20Repository: Openpanel-dev/openpanel
Length of output: 400
🏁 Script executed:
# Check if WEB_VERSION environment variable is set in build scripts
rg -n "WEB_VERSION\s*=" . --type sh --type json --type ts | head -20Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
# Look at how the version is actually supposed to be injected
rg -n "process.env.WEB_VERSION" . --type ts -B 3 -A 3Repository: Openpanel-dev/openpanel
Length of output: 469
🏁 Script executed:
# Check GitHub workflows or CI configs that might set versions
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "WEB_VERSION\|SDK_VERSION" 2>/dev/null | head -5Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
# Verify what happens in the minified bundle - check if process.env fallback is actually there
python3 << 'EOF'
# The minified bundle shows: t.sdkVersion||process.env.SDK_VERSION
# This means if t.sdkVersion is falsy, it accesses process.env.SDK_VERSION
# Let's trace what sdkVersion is when the web SDK instantiates the base class
code = """
In packages/sdks/web/src/index.ts:
sdkVersion: process.env.WEB_VERSION!,
In packages/sdks/sdk/src/index.ts:
defaultHeaders['openpanel-sdk-version'] = options.sdkVersion || process.env.SDK_VERSION!;
"""
print("ANALYSIS:")
print("1. Web SDK passes: sdkVersion = process.env.WEB_VERSION!")
print("2. If WEB_VERSION is undefined at build time, sdkVersion becomes undefined")
print("3. Undefined is falsy, so parent class evaluates: process.env.SDK_VERSION")
print("4. In browser, process is undefined -> ReferenceError")
print()
print("CONCLUSION: Review comment is CORRECT - unguarded process.env access in browser")
EOFRepository: Openpanel-dev/openpanel
Length of output: 417
Guard process.env access or provide a fallback for SDK version.
The base SDK class (packages/sdks/sdk/src/index.ts, line 56) accesses process.env.SDK_VERSION without checking if process is defined, which throws a ReferenceError in browsers if sdkVersion option is not provided. The web SDK attempts to pass process.env.WEB_VERSION! but this environment variable is not injected at build time in the tsup configuration, leaving the fallback unguarded.
Either guard the fallback with a default value:
defaultHeaders['openpanel-sdk-version'] =
options.sdkVersion || process.env.SDK_VERSION || '';Or inject WEB_VERSION in the tsup build config alongside __OPENPANEL_REPLAY_URL__.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/public/public/op1.js` at line 1, The SDK reads process.env.SDK_VERSION
unguarded in the base class constructor (class l -> constructor) when building
defaultHeaders for new c, which throws in browsers; update the default
sdkVersion fallback to prefer options.sdkVersion and only read
process.env.SDK_VERSION when process exists (e.g. check typeof
process!=="undefined" and process.env) and otherwise use an empty string as the
final fallback so defaultHeaders['openpanel-sdk-version'] never accesses a
missing global.
Comment on lines
+151
to
+161
| cell({ row }) { | ||
| const { sessionId } = row.original; | ||
| return ( | ||
| <ProjectLink | ||
| href={`/sessions/${encodeURIComponent(sessionId)}`} | ||
| className="whitespace-nowrap font-medium hover:underline" | ||
| > | ||
| {sessionId.slice(0,6)} | ||
| </ProjectLink> | ||
| ); | ||
| }, |
Contributor
There was a problem hiding this comment.
Guard sessionId before encoding/slicing to avoid row render crashes.
On Line 155 and Line 158, sessionId is assumed to be always present. If it’s null/undefined for any record, this will throw and break table rendering.
Suggested fix
cell({ row }) {
- const { sessionId } = row.original;
+ const { sessionId } = row.original;
+ if (!sessionId) {
+ return null;
+ }
+ const SESSION_ID_PREVIEW_LENGTH = 6;
return (
<ProjectLink
href={`/sessions/${encodeURIComponent(sessionId)}`}
className="whitespace-nowrap font-medium hover:underline"
>
- {sessionId.slice(0,6)}
+ {sessionId.slice(0, SESSION_ID_PREVIEW_LENGTH)}
</ProjectLink>
);
},As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cell({ row }) { | |
| const { sessionId } = row.original; | |
| return ( | |
| <ProjectLink | |
| href={`/sessions/${encodeURIComponent(sessionId)}`} | |
| className="whitespace-nowrap font-medium hover:underline" | |
| > | |
| {sessionId.slice(0,6)} | |
| </ProjectLink> | |
| ); | |
| }, | |
| cell({ row }) { | |
| const { sessionId } = row.original; | |
| if (!sessionId) { | |
| return null; | |
| } | |
| const SESSION_ID_PREVIEW_LENGTH = 6; | |
| return ( | |
| <ProjectLink | |
| href={`/sessions/${encodeURIComponent(sessionId)}`} | |
| className="whitespace-nowrap font-medium hover:underline" | |
| > | |
| {sessionId.slice(0, SESSION_ID_PREVIEW_LENGTH)} | |
| </ProjectLink> | |
| ); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/start/src/components/events/table/columns.tsx` around lines 151 - 161,
The cell renderer uses row.original.sessionId without guarding it, which can
throw when null/undefined; update the cell({ row }) implementation in the same
component to first safely read sessionId from row.original, return a fallback
(e.g., a placeholder or null element) when sessionId is falsy, and only call
encodeURIComponent and sessionId.slice when sessionId is a non-empty string;
also replace the magic number 6 used in sessionId.slice(0,6) with a named
constant (e.g., SESSION_ID_PREVIEW_LEN) declared near the component for clarity.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/sdks/web/src/index.ts (2)
142-144:⚠️ Potential issue | 🟠 MajorUse the injected
__OPENPANEL_REPLAY_URL__value when building the replay script URL.Line 144 checks that
__OPENPANEL_REPLAY_URL__is defined but never reads its value. The build-time replay URL configuration is effectively ignored.🔧 Proposed fix
if (typeof __OPENPANEL_REPLAY_URL__ !== 'undefined') { const scriptEl = _replayScriptRef; - const url = this.options.sessionReplay?.scriptUrl || scriptEl?.src?.replace('.js', '-replay.js') || 'https://openpanel.dev/op1-replay.js'; + const url = + this.options.sessionReplay?.scriptUrl ?? + __OPENPANEL_REPLAY_URL__ ?? + scriptEl?.src?.replace('.js', '-replay.js') ?? + 'https://openpanel.dev/op1-replay.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/index.ts` around lines 142 - 144, The code checks for the injected build-time constant __OPENPANEL_REPLAY_URL__ but never uses it when constructing the replay URL; update the URL resolution in the block around _replayScriptRef and the url variable to prefer the value of __OPENPANEL_REPLAY_URL__ when it is defined, falling back to this.options.sessionReplay?.scriptUrl, then to scriptEl?.src?.replace('.js','-replay.js'), and finally the hardcoded default; ensure references to __OPENPANEL_REPLAY_URL__, _replayScriptRef, and this.options.sessionReplay?.scriptUrl are used in that precedence.
102-118:⚠️ Potential issue | 🟡 MinorSampling decision is made on every page load, causing inconsistent replay sessions.
Math.random() < sampleRateis evaluated on each page load, so a user might be sampled on one page but not on subsequent navigations in the same session. Consider persisting the sampling decision tosessionStoragefor consistent per-session behavior.💡 Suggested approach
if (this.options.sessionReplay?.enabled) { const sampleRate = this.options.sessionReplay.sampleRate ?? 1; - const sampled = Math.random() < sampleRate; + const REPLAY_SAMPLED_KEY = 'openpanel-replay-sampled'; + let sampled: boolean; + try { + const stored = sessionStorage.getItem(REPLAY_SAMPLED_KEY); + if (stored !== null) { + sampled = stored === 'true'; + } else { + sampled = Math.random() < sampleRate; + sessionStorage.setItem(REPLAY_SAMPLED_KEY, String(sampled)); + } + } catch { + sampled = Math.random() < sampleRate; + } if (sampled) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdks/web/src/index.ts` around lines 102 - 118, The sampling decision for session replay should be made once per browser session and persisted so it remains consistent across page loads; update the logic around this.options.sessionReplay.sampleRate to first read a sessionStorage key (e.g., "sessionReplaySampled"), if absent compute sampled = Math.random() < sampleRate, store that boolean in sessionStorage, and then use that persisted value to decide whether to call loadReplayModule() and startReplayRecorder(this.options.sessionReplay!, ...); ensure the code reads/writes the same sessionStorage key so subsequent navigations in the same session reuse the decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/sdks/web/src/index.ts`:
- Around line 142-144: The code checks for the injected build-time constant
__OPENPANEL_REPLAY_URL__ but never uses it when constructing the replay URL;
update the URL resolution in the block around _replayScriptRef and the url
variable to prefer the value of __OPENPANEL_REPLAY_URL__ when it is defined,
falling back to this.options.sessionReplay?.scriptUrl, then to
scriptEl?.src?.replace('.js','-replay.js'), and finally the hardcoded default;
ensure references to __OPENPANEL_REPLAY_URL__, _replayScriptRef, and
this.options.sessionReplay?.scriptUrl are used in that precedence.
- Around line 102-118: The sampling decision for session replay should be made
once per browser session and persisted so it remains consistent across page
loads; update the logic around this.options.sessionReplay.sampleRate to first
read a sessionStorage key (e.g., "sessionReplaySampled"), if absent compute
sampled = Math.random() < sampleRate, store that boolean in sessionStorage, and
then use that persisted value to decide whether to call loadReplayModule() and
startReplayRecorder(this.options.sessionReplay!, ...); ensure the code
reads/writes the same sessionStorage key so subsequent navigations in the same
session reuse the decision.
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
New Features
Bug Fixes
Documentation