feat: Session replays - #563
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds session replay across stack: tracker recorder and uploads; backend ingest, S3 storage and signed requests, ClickHouse schema, MP4 export worker, web replays list/player/export, project retention, billing add-ons, docs, and packaging/runtime tweaks. ChangesSession Replay feature
Estimated code review effort Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin/src/billing/pricing.ts (1)
42-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync this entitlement table with the backend copy.
admin/src/billing/pricing.tsandbackend/apps/cloud/src/user/entities/user.entity.tsalready disagree onapiRateLimitPerHourforstandardandplus(600/5000here vs300/6000there). Anything that renders limits from this file can now advertise different numbers than the cloud backend serializes and enforces.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@admin/src/billing/pricing.ts` around lines 42 - 64, The planEntitlements constants in planEntitlements (referencing PlanType.standard and PlanType.plus) are out of sync with the backend: update apiRateLimitPerHour for PlanType.standard to 300 and for PlanType.plus to 6000 so this frontend/admin source matches backend/apps/cloud/src/user/entities/user.entity.ts; change the numeric values in the planEntitlements object and run or update any snapshot/tests that assert these advertised limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 1824-1841: The quota enforcement is race-prone because you SET NX
a per-replay reservation key and then separately call
getMonthlyReplayUsage/getReplayAccountProjectIds to decide if reservation should
stand; concurrent requests can all pass the read and over-admit. Replace the
two-step logic with an atomic Redis operation (a Lua EVAL script or a single
Redis command) that reads the current monthly usage for the account/project(s),
compares it to getSessionReplayQuota(project.admin), and only sets the
reservation key (and/or increments a usage counter) if usage < quota; reference
the current symbols: redis.set(key, ...), getReplayUsageTtlSeconds(),
getReplayAccountProjectIds(), getMonthlyReplayUsage(), getSessionReplayQuota(),
and ensure the script rolls back/does not create the reservation when quota
would be exceeded (or increments a counter atomically so removals/del logic
stays consistent).
- Around line 1903-1928: The current clickhouse.insert call in
analytics.service.ts writes chunks append-only which allows duplicate rows for
the same (pid, psid, replayId, chunkIndex); make ingestion idempotent by
enforcing uniqueness on that chunk key. Fix by changing the ClickHouse table
engine to a deduplicating engine (e.g., ReplacingMergeTree or
CollapsingMergeTree) with PRIMARY KEY (pid, psid, replayId, chunkIndex) and a
version/created column so duplicates are collapsed, and/or add a lightweight
pre-insert existence check in the code path that calls clickhouse.insert to skip
inserting if a row already exists for the same keys; update the
clickhouse.insert usage in analytics.service.ts to include the version/created
field used for dedupe. Ensure the unique key names (pid, psid, replayId,
chunkIndex) and the clickhouse.insert call are the targets of this change.
In `@backend/apps/cloud/src/analytics/dto/session-replay.dto.ts`:
- Around line 100-109: Update the GetSessionReplaysDto so pagination fields are
optional: add `@IsOptional`() to the take and skip properties (which are currently
decorated with `@Type`(() => Number), `@IsInt`(), `@Min`(0) and `@Max`(150) for take) so
omitted query params pass validation and the controller’s getSafeNumber defaults
can apply; also tighten replayId by adding `@IsNotEmpty`() (in addition to its
existing `@IsOptional`(), `@IsString`(), `@MaxLength`(80)) to reject empty strings as
invalid identities.
- Around line 33-35: SessionReplayStartDto.replayId currently allows empty
strings which can cause storage key collisions when SessionReplayChunkDto (which
extends it) forwards replayId into startSessionReplay/storeSessionReplayChunk;
add a non-empty check to the DTO by applying a class-validator constraint such
as `@IsNotEmpty`() (or `@MinLength`(1)) alongside the existing `@IsString`() and
`@MaxLength`(80), and ensure the decorator is imported (from class-validator) so
replayId cannot be ''.
In `@backend/apps/cloud/src/analytics/session-replay-export.service.ts`:
- Around line 385-387: The code currently buffers the entire MP4 via
fs.readFile(mp4Path) before calling sessionReplayStorage.putObject, which can
OOM for large exports; change it to stream the file instead: replace
fs.readFile(mp4Path) with a readable stream created from mp4Path (e.g.,
fs.createReadStream(mp4Path)) and pass that stream to
sessionReplayStorage.putObject (or use the storage client's streaming/multipart
upload helper) while still providing the same objectKey from getObjectKey and
content type 'video/mp4' so the file is uploaded without materializing the whole
Buffer in memory.
- Around line 389-414: The export is already marked ready and objectKey set by
updateState, but a failure in exportQueue.add causes the outer catch to call
markFailed and clear the objectKey, making a valid export unavailable; instead,
wrap the exportQueue.add call in its own try/catch so that failures to schedule
cleanup do not flip the export to failed or clear objectKey—call
updateState/status and persist objectKey only once (updateState(... status:
'ready', objectKey ...)), and if exportQueue.add throws, log the error and
optionally enqueue a retry or alert, but do not call markFailed or modify
objectKey; reference the existing updateState, exportQueue.add, markFailed,
objectKey, and status:'ready' symbols when making the change.
- Around line 429-441: The current inline .catch on
this.sessionReplayStorage.deleteObject silences R2 delete failures so the job is
marked successful and expireState clears state.objectKey; change this so
failures propagate and only expire the state when deletion succeeds: remove the
.catch (or rethrow after logging) from deleteObject call on sessionReplayStorage
(referencing deleteObject, sessionReplayStorage, and data.exportId) and ensure
expireState(state) is invoked only after the deleteObject promise resolves
successfully (so expireState is not called when delete fails).
In `@backend/apps/cloud/src/analytics/session-replay-r2.service.ts`:
- Around line 181-185: The fetch call that performs R2 storage operations (the
invocation using url, method, headers, requestBody in
session-replay-r2.service.ts) needs an explicit request timeout: create an
AbortController, pass controller.signal in the fetch options, schedule a
setTimeout to call controller.abort() after a sensible timeout (e.g., 30s), and
clear the timeout once fetch resolves/rejects so you don’t leak timers; ensure
any AbortError is handled/propagated consistently by the existing caller logic.
In `@backend/apps/cloud/src/user/entities/user.entity.ts`:
- Around line 218-234: The helper getSessionReplayRetentionEntitlement relies on
getEffectivePlanType(user) which can return null when planCode is missing, so
passing only { planType: PlanType.enterprise } currently yields the wrong
default; fix by first honoring an explicit user.planType (e.g., if
user?.planType === PlanType.enterprise return 1825), then fall back to
getEffectivePlanType(user) for determination, and optionally update the user
input type or call sites to include planCode if you want getEffectivePlanType to
be authoritative; reference getSessionReplayRetentionEntitlement,
getEffectivePlanType, and PlanType when making the change.
- Around line 95-108: PLUS_SESSION_REPLAY_QUOTA is missing an entry for the
legacy PlanCode.freelancer, so getSessionReplayQuota() falls back to 0 for those
users; add a mapping for PlanCode.freelancer with the same value as the 100k
tier in PLUS_SESSION_REPLAY_QUOTA (i.e., mirror the value used for
PlanCode['100k']) so legacy freelancer plus plans receive the correct replay
quota.
In `@packages/tracker-js/README.md`:
- Around line 197-223: The README and the script reference disagree on the
default privacy for startSessionReplay (README says 'total', script reference
says 'normal'); inspect the actual implementation of startSessionReplay to
determine the true default, then update the documentation so both the README
entry for startSessionReplay (example and table) and the script-reference doc
entry use that verified default value and wording; ensure the `privacy` default
is consistent in the function docs, table row, and any example code.
In `@packages/tracker-js/src/Lib.ts`:
- Around line 803-805: Validate runtime privacy input before using it: replace
the direct use of options.privacy (and the similar block around lines 1149-1179)
with a whitelist check against the allowed privacy constants/enum and only
accept exact matches; if options.privacy is missing or not one of the allowed
values, explicitly set privacy = DEFAULT_SESSION_REPLAY_PRIVACY (the safe
default) so typos like "totl" cannot fall through to a less-private behavior;
apply this validation where replayId is created and before calling
this.sendSessionReplayStart and any other call sites that consume privacy.
- Around line 1113-1130: The current rrweb loader Promise stored in
window.__SWETRIX_RRWEB_LOADING__ and this.rrwebLoader is left rejected on a load
failure, preventing retries; modify the loader creation in Lib.ts so that on
script.onerror (or promise rejection) you clear the global and instance
references (delete window.__SWETRIX_RRWEB_LOADING__ and set this.rrwebLoader =
undefined/null) before rejecting so subsequent startSessionReplay() calls can
attempt to load again; locate the loader assignment around
this.getSessionReplayUrl(), window.__SWETRIX_RRWEB_LOADING__, and
this.rrwebLoader to implement the change.
In `@web/app/pages/Project/Settings/tabs/SessionReplays.tsx`:
- Around line 80-87: The radio inputs in SessionReplays.tsx lack a shared name,
so they don't behave as a single radio group; update the <input> inside the map
(the element using props/variables days, label, className and the onChange
handler onRetentionChange) to include a common name attribute (e.g.,
name="retention" or a constant like RETENTION_RADIO_NAME) so all retention
options share the same radio group and restore native keyboard behavior.
In `@web/app/pages/Project/tabs/Sessions/SessionReplayModal.tsx`:
- Around line 1388-1394: The modal close handler currently clears the timeout
via clearExportPoll but doesn't prevent an in-flight poll request from resolving
and scheduling another timeout; update the polling flow (the pollExportStatus
function and any place that schedules setTimeout) to check a modal-open guard
before scheduling the next poll or showing toasts, and cancel/ignore in-flight
fetches using an AbortController or an "isOpen" ref (e.g., isModalOpenRef) that
is set false on close; also ensure clearExportPoll aborts the controller or
flips the ref so resolved promises do nothing. Apply the same guard/abort logic
to the other polling blocks referenced (around the functions used at 1678-1704
and 1761-1771).
In `@web/app/routes/api.session-replay-export.ts`:
- Around line 29-40: The handler currently calls request.json() directly into
the body variable which will throw on malformed JSON and produce a 500; wrap the
JSON parsing in a try/catch around the request.json() call (the body variable)
and return a 400 ProxyResponse when parsing fails with a clear error message
before performing the existing checks for projectId/psid. Keep the existing
validation logic for body.projectId and body.psid (the subsequent conditional)
but ensure it only runs when parsing succeeded.
---
Outside diff comments:
In `@admin/src/billing/pricing.ts`:
- Around line 42-64: The planEntitlements constants in planEntitlements
(referencing PlanType.standard and PlanType.plus) are out of sync with the
backend: update apiRateLimitPerHour for PlanType.standard to 300 and for
PlanType.plus to 6000 so this frontend/admin source matches
backend/apps/cloud/src/user/entities/user.entity.ts; change the numeric values
in the planEntitlements object and run or update any snapshot/tests that assert
these advertised limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39e13fe8-bd53-43c2-9e5a-0c055560768d
⛔ Files ignored due to path filters (3)
backend/package-lock.jsonis excluded by!**/package-lock.jsonpackages/tracker-js/package-lock.jsonis excluded by!**/package-lock.jsonweb/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (75)
admin/src/billing/pricing.tsbackend/.env.examplebackend/Dockerfilebackend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.module.tsbackend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/bot-detection.service.tsbackend/apps/cloud/src/analytics/dto/session-replay.dto.tsbackend/apps/cloud/src/analytics/session-replay-export.processor.tsbackend/apps/cloud/src/analytics/session-replay-export.service.tsbackend/apps/cloud/src/analytics/session-replay-r2.service.tsbackend/apps/cloud/src/logger/logger.service.tsbackend/apps/cloud/src/main.tsbackend/apps/cloud/src/project/dto/update-project.dto.tsbackend/apps/cloud/src/project/entity/project.entity.tsbackend/apps/cloud/src/project/project.controller.tsbackend/apps/cloud/src/project/project.service.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/apps/cloud/src/user/entities/user.entity.tsbackend/apps/cloud/src/user/user.service.tsbackend/knip.jsoncbackend/migrations/clickhouse/2026_06_03_session_replays.jsbackend/migrations/clickhouse/initialise_database.jsbackend/migrations/mysql/2026_06_03_session_replays.sqlbackend/package.jsonbackend/scripts/patch-rrvideo.jsdocs/content/docs/add-script.mdxdocs/content/docs/analytics-dashboard/error-tracking.mdxdocs/content/docs/analytics-dashboard/meta.jsondocs/content/docs/analytics-dashboard/profiles-and-sessions.mdxdocs/content/docs/analytics-dashboard/session-replays.mdxdocs/content/docs/script-reference.mdxdocs/content/docs/sitesettings/project-configuration.mdxpackages/tracker-js/README.mdpackages/tracker-js/package.jsonpackages/tracker-js/rollup.config.mjspackages/tracker-js/src/Lib.tspackages/tracker-js/src/index.tspackages/tracker-js/tests/sessionReplay.test.tspackages/tracker-js/tsconfig.esnext.jsonpackages/tracker-js/tsconfig.jsonweb/app/api/api.server.tsweb/app/components/marketing/FeaturesGrid.tsxweb/app/components/pricing/MarketingPricing.tsxweb/app/components/pricing/SessionReplayPricingTooltip.tsxweb/app/hooks/useAnalyticsProxy.tsweb/app/lib/constants/index.tsweb/app/lib/models/Project.tsweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/SettingsSidebar.tsxweb/app/pages/Project/Settings/tabs/SessionReplays.tsxweb/app/pages/Project/View/ViewProject.tsxweb/app/pages/Project/View/components/ProjectSidebar.tsxweb/app/pages/Project/View/utils/projectViewSegments.tsweb/app/pages/Project/tabs/Replays/NoReplays.tsxweb/app/pages/Project/tabs/Replays/Replays.tsxweb/app/pages/Project/tabs/Replays/ReplaysView.tsxweb/app/pages/Project/tabs/Sessions/Pageflow.tsxweb/app/pages/Project/tabs/Sessions/SessionDetailView.tsxweb/app/pages/Project/tabs/Sessions/SessionReplayModal.tsxweb/app/pages/Project/tabs/Sessions/Sessions.tsxweb/app/pages/Project/tabs/Sessions/SessionsView.tsxweb/app/pages/Project/tabs/Sessions/replayPlayer.cssweb/app/pages/Subscribe/Subscribe.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/routes/_index.tsxweb/app/routes/api.analytics.tsweb/app/routes/api.session-replay-export.tsweb/app/routes/projects.$id.tsxweb/app/routes/projects.settings.$id.tsxweb/app/types/dom-player.d.tsweb/knip.jsoncweb/package.jsonweb/public/locales/en.jsonweb/vite.config.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
admin/src/billing/pricing.ts (1)
66-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd legacy
freelancertier to admin replay quotas.
sessionReplayQuotasomitsPlanCode.freelancer, so plus users on that legacy 100k SKU resolve to0ingetEffectiveLimits. Backend quota logic mapsfreelancerto5000(backend/apps/cloud/src/user/entities/user.entity.ts, Line 95-98), so admin and backend currently diverge.Suggested fix
const sessionReplayQuotas: Partial<Record<PlanCode, number>> = { + [PlanCode.freelancer]: 5000, [PlanCode['100k']]: 5000, [PlanCode['200k']]: 10000, [PlanCode['500k']]: 25000,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@admin/src/billing/pricing.ts` around lines 66 - 79, sessionReplayQuotas is missing the legacy PlanCode.freelancer entry so getEffectiveLimits returns 0 for those users; add a freelancer key to sessionReplayQuotas with the same quota as the 100k SKU (5000) to match backend behavior. Locate the sessionReplayQuotas object in admin/src/billing/pricing.ts and add [PlanCode.freelancer]: 5000 (matching backend/apps/cloud/src/user/entities/user.entity.ts mapping) so admin and backend quotas align.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@admin/src/billing/pricing.ts`:
- Around line 66-79: sessionReplayQuotas is missing the legacy
PlanCode.freelancer entry so getEffectiveLimits returns 0 for those users; add a
freelancer key to sessionReplayQuotas with the same quota as the 100k SKU (5000)
to match backend behavior. Locate the sessionReplayQuotas object in
admin/src/billing/pricing.ts and add [PlanCode.freelancer]: 5000 (matching
backend/apps/cloud/src/user/entities/user.entity.ts mapping) so admin and
backend quotas align.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebd196b4-7763-4a1b-88b4-e1cd681f7d05
📒 Files selected for processing (16)
admin/src/billing/pricing.tsbackend/.env.examplebackend/apps/cloud/src/analytics/analytics.module.tsbackend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/dto/session-replay.dto.tsbackend/apps/cloud/src/analytics/session-replay-export.service.tsbackend/apps/cloud/src/analytics/session-replay-s3.service.tsbackend/apps/cloud/src/user/entities/user.entity.tsdocs/content/docs/script-reference.mdxpackages/tracker-js/src/Lib.tspackages/tracker-js/tests/sessionReplay.test.tsweb/app/hooks/useAnalyticsProxy.tsweb/app/pages/Project/Settings/tabs/SessionReplays.tsxweb/app/pages/Project/tabs/Sessions/SessionReplayModal.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/routes/api.session-replay-export.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- web/app/pages/Project/Settings/tabs/SessionReplays.tsx
- web/app/hooks/useAnalyticsProxy.ts
- web/app/routes/api.session-replay-export.ts
- web/app/pages/Project/tabs/Sessions/SessionReplayModal.tsx
- docs/content/docs/script-reference.mdx
- backend/apps/cloud/src/analytics/dto/session-replay.dto.ts
- web/app/pages/UserSettings/UserSettings.tsx
- packages/tracker-js/tests/sessionReplay.test.ts
- backend/apps/cloud/src/analytics/session-replay-export.service.ts
- packages/tracker-js/src/Lib.ts
- backend/apps/cloud/src/analytics/analytics.service.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/apps/cloud/src/project/project.service.ts (1)
1032-1074: ⚡ Quick winCache monthly replay usage to avoid repeated
uniqExactscans.
GET /user/usageinfonow calls this path directly, and this method always hits ClickHouse. Adding Redis caching (same TTL strategy as other usage methods) would reduce expensive repeated scans.♻️ Suggested change
async getMonthlySessionReplayUsage(uid: string): Promise<number> { + const key = `${getRedisUserUsageInfoKey(uid)}_sessionReplays` + const cached = await redis.get(key) + if (!_isEmpty(cached)) { + const parsed = Number(cached) + if (Number.isFinite(parsed)) { + return parsed + } + } + const projects = await this.find({ where: { admin: { id: uid }, }, select: ['id'], @@ for (let i = 0; i < pids.length; i += CHUNK_SIZE) { @@ usage += Number(data[0]?.usage) || 0 } + await redis.set(key, `${usage}`, 'EX', redisUserUsageinfoCacheTimeout) return usage }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/project/project.service.ts` around lines 1032 - 1074, The getMonthlySessionReplayUsage method currently always queries ClickHouse; add Redis caching around it using the same TTL strategy as other usage methods: compose a stable cache key (e.g., `monthly_replay_usage:{uid}:{YYYY-MM}`) at the start of getMonthlySessionReplayUsage, try to read and return the cached numeric value if present, and only if missing run the existing CHUNK_SIZE loop/clickhouse.query logic to compute usage, then store the computed value back into Redis with the same TTL used elsewhere before returning; ensure you use the same Redis client/service the project already uses so you don’t introduce a new dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/cloud/src/task-manager/task-manager.service.ts`:
- Around line 1455-1458: The parallel invocation of
processDueWebsiteAddonRenewals and processDueSessionReplayAddonRenewals via
Promise.all can concurrently mutate the same user's addon override state and
cause races; change the TaskManagerService call site to run them sequentially
instead of in Promise.all (await
this.userService.processDueWebsiteAddonRenewals() then await
this.userService.processDueSessionReplayAddonRenewals()), preserving the
existing catch/error handling, or alternatively implement row-level
merge/locking inside the userService methods if you prefer a concurrency-safe
merge strategy; update the call where both functions are referenced to ensure
one completes before the next starts.
In `@backend/migrations/mysql/2026_06_04_session_replay_addons.sql`:
- Around line 1-2: The migration alters user_addon.code enum via ALTER TABLE
`user_addon` MODIFY COLUMN `code` enum('websites','session_replays') NOT NULL
which can fail if existing rows contain NULL or any value outside the new enum
set; update the PR description to state this is a schema change (ENUM update +
column definition) and, before deploying, run a data validation query to find
non-conforming rows (WHERE code IS NULL OR code NOT IN
('websites','session_replays')), then fix them by either mapping/fixing values,
setting a valid default, or removing offending rows prior to running the ALTER
TABLE so the enum change cannot fail at runtime.
---
Nitpick comments:
In `@backend/apps/cloud/src/project/project.service.ts`:
- Around line 1032-1074: The getMonthlySessionReplayUsage method currently
always queries ClickHouse; add Redis caching around it using the same TTL
strategy as other usage methods: compose a stable cache key (e.g.,
`monthly_replay_usage:{uid}:{YYYY-MM}`) at the start of
getMonthlySessionReplayUsage, try to read and return the cached numeric value if
present, and only if missing run the existing CHUNK_SIZE loop/clickhouse.query
logic to compute usage, then store the computed value back into Redis with the
same TTL used elsewhere before returning; ensure you use the same Redis
client/service the project already uses so you don’t introduce a new dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a015d119-4548-4737-a120-e37cf0a10e17
📒 Files selected for processing (17)
backend/apps/cloud/src/project/project.service.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/apps/cloud/src/user/dto/update-website-addon.dto.tsbackend/apps/cloud/src/user/entities/user-addon.entity.tsbackend/apps/cloud/src/user/entities/user.entity.tsbackend/apps/cloud/src/user/interfaces/usage-info.tsbackend/apps/cloud/src/user/user.controller.tsbackend/apps/cloud/src/user/user.service.tsbackend/apps/cloud/src/webhook/webhook.controller.tsbackend/migrations/mysql/2026_06_04_session_replay_addons.sqlweb/app/lib/models/Usageinfo.tsweb/app/lib/models/User.tsweb/app/pages/Project/tabs/Replays/Replays.tsxweb/app/pages/Project/tabs/Replays/ReplaysView.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/routes/user-settings.tsxweb/public/locales/en.json
💤 Files with no reviewable changes (1)
- web/app/pages/Project/tabs/Replays/ReplaysView.tsx
✅ Files skipped from review due to trivial changes (1)
- backend/apps/cloud/src/user/entities/user-addon.entity.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/cloud/src/analytics/analytics.service.ts (1)
2208-2225:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
argMax(..., created)is still ambiguous for same-second duplicate rows.The new read-side dedupe picks the “latest” chunk metadata by
created, but inserts still roundcreatedtoYYYY-MM-DD HH:mm:ss. Two writes of the same chunk within one second will share the same sort key, sogetSessionReplaySummary(),getSessionReplay(), andgetSessionReplaysList()can still select stale metadata nondeterministically. This needs a strictly monotonic version column (for exampleDateTime64or epoch millis) and theargMaxcalls should switch to that key.Also applies to: 2266-2282, 2346-2355, 6743-6759
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 2208 - 2225, The insert uses a seconds-granularity "created" timestamp (dayjs.utc().format('YYYY-MM-DD HH:mm:ss')) which allows duplicate sort keys for writes within the same second; change the insert into session_replay_chunks (and the other similar inserts) to include a strictly monotonic version column (e.g., createdMillis as epoch milliseconds or a DateTime64 value) alongside the existing created field, and update all read-side argMax(...) calls (used by getSessionReplaySummary, getSessionReplay, getSessionReplaysList) to use this new createdMillis/version column as the sort key so argMax is deterministic for same-second duplicate rows.
♻️ Duplicate comments (1)
backend/apps/cloud/src/analytics/analytics.service.ts (1)
2089-2100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat duplicate chunk retries as idempotent success.
When the same
chunkIndexis retried after the original upload already succeeded, this path now returns400instead of a no-op success. That still breaks safe client retries on lost responses/timeouts, even though the duplicate write is suppressed.🛠️ Possible direction
- case -4: - throw new BadRequestException('Session replay chunk already exists') + case -4: + return null- const chunkReservation = await this.reserveReplayChunkStorage( + const chunkReservation = await this.reserveReplayChunkStorage( pid, psid, replayId, chunkIndex, events.length, uncompressedBytes, retention, timestamps, ) + if (!chunkReservation) { + return { + replayId, + psid, + chunkIndex, + eventCount: events.length, + ...retention, + countedUsage: false, + } + }Also applies to: 2241-2248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 2089 - 2100, The switch on Number(result) currently throws BadRequestException for duplicate chunk result (-5); change the -5 branch to treat duplicate chunk retries as idempotent success by returning the same success payload as case 1 (use the existing variables keys, chunkIndex, eventCount, uncompressedBytes) instead of throwing; apply the same change to the other equivalent switch block (the similar handling at the 2241-2248 region) so client retries receive a no-op success rather than 400.
🧹 Nitpick comments (1)
packages/tracker-js/tests/sessionReplay.test.ts (1)
297-303: ⚡ Quick winAvoid asserting that retries leave failed loader scripts behind.
expect(scripts).toHaveLength(2)locks the test to the current DOM leak/implementation detail. If the loader starts removing failed<script>nodes, retry behavior is still correct but this suite will fail. Assert the cleared retry state and successful reload instead of the exact stale node count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tracker-js/tests/sessionReplay.test.ts` around lines 297 - 303, Remove the brittle assertion that checks the DOM node count (expect(scripts).toHaveLength(2)) and instead verify the retry machinery and success path: after calling startSessionReplay() and simulating the rrweb load (using startSessionReplay, RRWEB_URL, scripts and scripts[1].dispatchEvent(new Event('load'))), assert that the retry state has been cleared (e.g. any retry counter or flag set by startSessionReplay is reset) and that the reload succeeded (e.g. rrweb.record is present or the success callback/state the loader sets is true) rather than asserting the exact number of <script> nodes. Ensure you reference the same symbols (startSessionReplay, RRWEB_URL, scripts, rrweb) when updating the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 2208-2225: The insert uses a seconds-granularity "created"
timestamp (dayjs.utc().format('YYYY-MM-DD HH:mm:ss')) which allows duplicate
sort keys for writes within the same second; change the insert into
session_replay_chunks (and the other similar inserts) to include a strictly
monotonic version column (e.g., createdMillis as epoch milliseconds or a
DateTime64 value) alongside the existing created field, and update all read-side
argMax(...) calls (used by getSessionReplaySummary, getSessionReplay,
getSessionReplaysList) to use this new createdMillis/version column as the sort
key so argMax is deterministic for same-second duplicate rows.
---
Duplicate comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 2089-2100: The switch on Number(result) currently throws
BadRequestException for duplicate chunk result (-5); change the -5 branch to
treat duplicate chunk retries as idempotent success by returning the same
success payload as case 1 (use the existing variables keys, chunkIndex,
eventCount, uncompressedBytes) instead of throwing; apply the same change to the
other equivalent switch block (the similar handling at the 2241-2248 region) so
client retries receive a no-op success rather than 400.
---
Nitpick comments:
In `@packages/tracker-js/tests/sessionReplay.test.ts`:
- Around line 297-303: Remove the brittle assertion that checks the DOM node
count (expect(scripts).toHaveLength(2)) and instead verify the retry machinery
and success path: after calling startSessionReplay() and simulating the rrweb
load (using startSessionReplay, RRWEB_URL, scripts and
scripts[1].dispatchEvent(new Event('load'))), assert that the retry state has
been cleared (e.g. any retry counter or flag set by startSessionReplay is reset)
and that the reload succeeded (e.g. rrweb.record is present or the success
callback/state the loader sets is true) rather than asserting the exact number
of <script> nodes. Ensure you reference the same symbols (startSessionReplay,
RRWEB_URL, scripts, rrweb) when updating the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5c9b2cc-e428-4b90-b349-20982e7186c1
📒 Files selected for processing (13)
backend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/session-replay-export.service.tsbackend/apps/cloud/src/analytics/session-replay-s3.service.tsbackend/apps/cloud/src/project/project.service.tsbackend/apps/cloud/src/task-manager/task-manager.service.tsbackend/scripts/patch-rrvideo.jsdocs/content/docs/analytics-dashboard/session-replays.mdxpackages/tracker-js/rollup.config.mjspackages/tracker-js/src/Lib.tspackages/tracker-js/tests/sessionReplay.test.tsweb/app/lib/pricing/catalog.tsweb/app/pages/UserSettings/UserSettings.tsxweb/public/locales/en.json
💤 Files with no reviewable changes (1)
- web/app/lib/pricing/catalog.ts
✅ Files skipped from review due to trivial changes (2)
- docs/content/docs/analytics-dashboard/session-replays.mdx
- web/public/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/apps/cloud/src/project/project.service.ts
- backend/scripts/patch-rrvideo.js
- packages/tracker-js/src/Lib.ts
- backend/apps/cloud/src/analytics/session-replay-s3.service.ts
- backend/apps/cloud/src/task-manager/task-manager.service.ts
- web/app/pages/UserSettings/UserSettings.tsx
- backend/apps/cloud/src/analytics/session-replay-export.service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/app/pages/Project/tabs/Replays/ReplaysView.tsx`:
- Around line 304-315: removeReplayFromList currently always decrements
replaysSkip even when nothing was removed; change it so you detect if a replay
was actually removed (e.g., inside the setReplays callback compare prev.length
and next.length or check presence of removedReplayKey) and only call
setReplaysSkip(prev => Math.max(0, prev - 1)) when a removal occurred; update
hasShownContentRef.current based on the computed next list as you already do.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbd7f181-aa69-42d2-9b63-604e1f48e41b
📒 Files selected for processing (14)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.service.tspackages/tracker-js/tests/sessionReplay.test.tsweb/app/api/api.server.tsweb/app/hooks/useAnalyticsProxy.tsweb/app/pages/Project/Settings/tabs/SessionReplays.tsxweb/app/pages/Project/tabs/Replays/Replays.tsxweb/app/pages/Project/tabs/Replays/ReplaysView.tsxweb/app/pages/Project/tabs/Sessions/SessionDetailView.tsxweb/app/pages/Project/tabs/Sessions/SessionReplayModal.tsxweb/app/pages/Project/tabs/Sessions/Sessions.tsxweb/app/routes/api.analytics.tsweb/app/ui/Select.tsxweb/public/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (11)
- web/app/pages/Project/tabs/Sessions/Sessions.tsx
- web/app/routes/api.analytics.ts
- web/app/pages/Project/tabs/Replays/Replays.tsx
- web/app/pages/Project/tabs/Sessions/SessionDetailView.tsx
- web/app/hooks/useAnalyticsProxy.ts
- web/app/api/api.server.ts
- backend/apps/cloud/src/analytics/analytics.controller.ts
- web/app/pages/Project/tabs/Sessions/SessionReplayModal.tsx
- backend/apps/cloud/src/analytics/analytics.service.ts
- web/public/locales/en.json
- packages/tracker-js/tests/sessionReplay.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tracker-js/src/Lib.ts (1)
789-815:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize concurrent
startSessionReplay()calls.Line 789 only deduplicates after
this.sessionReplayActionsexists. Until Line 937, a second caller can run the same async path, mint anotherreplayId, and start a secondrrweb.record()instance. That duplicates stored chunks and can overcount replay usage/billing.Suggested fix
+ private sessionReplayStartPromise: Promise<SessionReplayActions> | null = null + async startSessionReplay( options: SessionReplayOptions = {}, ): Promise<SessionReplayActions> { if (this.sessionReplayActions) { return this.sessionReplayActions } + if (this.sessionReplayStartPromise) { + return this.sessionReplayStartPromise + } - if (!this.canTrack()) { - return defaultSessionReplayActions - } + const startPromise = (async (): Promise<SessionReplayActions> => { + if (!this.canTrack()) { + return defaultSessionReplayActions + } - if (!this.shouldSampleSessionReplay(options.sampleRate)) { - return defaultSessionReplayActions - } + if (!this.shouldSampleSessionReplay(options.sampleRate)) { + return defaultSessionReplayActions + } - try { - await this.preloadSessionReplay() - } catch { - return defaultSessionReplayActions - } + try { + await this.preloadSessionReplay() + } catch { + return defaultSessionReplayActions + } - // ... existing start logic ... + // ... existing start logic ... - this.sessionReplayActions = { - stop: stopSessionReplay, - flush: async () => { - await flush() - }, - } + this.sessionReplayActions = { + stop: stopSessionReplay, + flush: async () => { + await flush() + }, + } - return this.sessionReplayActions + return this.sessionReplayActions + })() + + this.sessionReplayStartPromise = startPromise + + try { + return await startPromise + } finally { + if (this.sessionReplayStartPromise === startPromise) { + this.sessionReplayStartPromise = null + } + } }Also applies to: 937-944
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tracker-js/src/Lib.ts` around lines 789 - 815, Concurrent callers to startSessionReplay() can race before this.sessionReplayActions is set, causing multiple replayIds and duplicate rrweb.record() instances; fix by serializing initialization: create and assign a single in-flight promise (e.g. this.sessionReplayInitPromise or set this.sessionReplayActions to a placeholder promise) at the very start of startSessionReplay() so subsequent calls await it, perform the async steps (preloadSessionReplay, sendSessionReplayStart, rrweb.record) only once, and on failure clear the promise so retries are possible; ensure the same guard is applied around the rrweb.record creation path (the code that calls window.rrweb.record and the block referenced around lines 937-944) to prevent a second recorder from starting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 2098-2099: The -4 switch branch in the reservation logic currently
returns the same reservation shape as a fresh reservation, allowing
storeSessionReplayChunk() to continue and later call
releaseReplayChunkStorageReservation() on an already-committed chunk; change the
-4 branch to explicitly mark the result as a duplicate (e.g., include a flag
like duplicate: true or status: 'duplicate') and short-circuit the flow so
upload/rollback steps are skipped; then update storeSessionReplayChunk() to
check that flag and return early (without calling
releaseReplayChunkStorageReservation()) when a duplicate is detected to avoid
removing the original chunk and decrementing counters.
- Around line 2417-2429: deleteSessionReplay currently deletes S3 objects via
sessionReplayStorage.deleteObject and then issues an asynchronous ClickHouse
DELETE through clickhouse.command on session_replay_chunks, causing races;
change the flow so the ClickHouse DELETE runs synchronously before removing S3
objects (or set mutations_sync=2 on the ALTER TABLE DELETE) to ensure rows are
gone before object deletion—update the call around clickhouse.command in
deleteSessionReplay to include mutations_sync=2 in the query params or move the
ALTER TABLE block to execute and wait (synchronously) prior to calling mapLimit
on sessionReplayStorage.deleteObject.
---
Outside diff comments:
In `@packages/tracker-js/src/Lib.ts`:
- Around line 789-815: Concurrent callers to startSessionReplay() can race
before this.sessionReplayActions is set, causing multiple replayIds and
duplicate rrweb.record() instances; fix by serializing initialization: create
and assign a single in-flight promise (e.g. this.sessionReplayInitPromise or set
this.sessionReplayActions to a placeholder promise) at the very start of
startSessionReplay() so subsequent calls await it, perform the async steps
(preloadSessionReplay, sendSessionReplayStart, rrweb.record) only once, and on
failure clear the promise so retries are possible; ensure the same guard is
applied around the rrweb.record creation path (the code that calls
window.rrweb.record and the block referenced around lines 937-944) to prevent a
second recorder from starting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d6d1e2f-9489-487e-a121-a8f9a855b479
📒 Files selected for processing (23)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.service.tsdocs/content/docs/add-script.mdxdocs/content/docs/analytics-dashboard/session-replays.mdxdocs/content/docs/script-reference.mdxpackages/tracker-js/README.mdpackages/tracker-js/rollup.config.mjspackages/tracker-js/src/Lib.tspackages/tracker-js/src/types/rrweb-shim.d.tspackages/tracker-js/tests/sessionReplay.test.tspackages/tracker-js/tsconfig.esnext.jsonpackages/tracker-js/tsconfig.jsonweb/app/api/api.server.tsweb/app/hooks/useAnalyticsProxy.tsweb/app/pages/Project/Settings/tabs/SessionReplays.tsxweb/app/pages/Project/tabs/Replays/Replays.tsxweb/app/pages/Project/tabs/Replays/ReplaysView.tsxweb/app/pages/Project/tabs/Sessions/SessionDetailView.tsxweb/app/pages/Project/tabs/Sessions/SessionReplayModal.tsxweb/app/pages/Project/tabs/Sessions/Sessions.tsxweb/app/routes/api.analytics.tsweb/app/ui/Select.tsxweb/public/locales/en.json
✅ Files skipped from review due to trivial changes (3)
- packages/tracker-js/src/types/rrweb-shim.d.ts
- docs/content/docs/add-script.mdx
- docs/content/docs/analytics-dashboard/session-replays.mdx
🚧 Files skipped from review as they are similar to previous changes (16)
- docs/content/docs/script-reference.mdx
- packages/tracker-js/tsconfig.json
- packages/tracker-js/README.md
- web/app/pages/Project/Settings/tabs/SessionReplays.tsx
- packages/tracker-js/rollup.config.mjs
- web/app/ui/Select.tsx
- web/app/hooks/useAnalyticsProxy.ts
- web/app/pages/Project/tabs/Sessions/SessionDetailView.tsx
- web/app/routes/api.analytics.ts
- web/app/pages/Project/tabs/Replays/ReplaysView.tsx
- web/app/pages/Project/tabs/Replays/Replays.tsx
- web/app/pages/Project/tabs/Sessions/SessionReplayModal.tsx
- web/app/pages/Project/tabs/Sessions/Sessions.tsx
- web/app/api/api.server.ts
- backend/apps/cloud/src/analytics/analytics.controller.ts
- web/public/locales/en.json
6 tasks
6 tasks
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.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Billing