Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Contributor
|
Size Change: +14 kB (+0.21%) Total Size: 6.59 MB
ℹ️ View Unchanged
|
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
5d6ec50 to
c5ce1cd
Compare
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
c5ce1cd to
755ee64
Compare
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
755ee64 to
5182796
Compare
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
5182796 to
673a1da
Compare
ksvat
requested review from
a team,
TueHaulund and
fasyy612
and removed request for
a team
ksvat
requested review from
a team,
TueHaulund,
fasyy612 and
pauldambra
and removed request for
a team
Contributor
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Line: 1686-1692
Comment:
**Cross-group trigger state contamination via shared persistence keys**
When any V2 group's event trigger fires, `_activateTrigger('event', ...)` is called, which writes `SESSION_RECORDING_EVENT_TRIGGER_ACTIVATED_SESSION = sessionId` to shared persistence. Every other group's `EventTriggerMatching._eventTriggerStatus()` reads that **same** shared key to determine its own activation state. The result is that activating group A via `$exception` will also cause every other group with event triggers to report `TRIGGER_ACTIVATED`, regardless of whether their own event conditions were met.
Concrete example: Group A has `events: ['$exception']`, Group B has `events: ['$pageview']`.
1. `$exception` fires → Group A matches → `_activateTrigger('event')` → `SESSION_RECORDING_EVENT_TRIGGER_ACTIVATED_SESSION = sessionId`
2. On the next status check, Group B's `EventTriggerMatching._eventTriggerStatus()` checks that same key, sees `sessionId`, and returns `TRIGGER_ACTIVATED`
3. Group B is incorrectly activated — and if Group B's sampling decision was `true`, recording starts as `SAMPLED` even though `$pageview` was never captured
The same cross-contamination applies to URL triggers via `SESSION_RECORDING_URL_TRIGGER_ACTIVATED_SESSION`.
To fix this, each group's trigger activation state needs to be tracked with a **group-scoped** persistence key (e.g. `SESSION_RECORDING_EVENT_TRIGGER_ACTIVATED_SESSION_${groupId}`) instead of the single global key, or the `TriggerGroupMatching.triggerStatus()` path needs to bypass the shared persistence lookup entirely and track activation in per-group local state.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Line: 413-424
Comment:
**`_minimumDuration` includes trigger-pending groups, not just activated ones**
The PR description states: *"Buffer flushes when duration meets the LOWEST `minDurationMs` among **activated** groups."* However the current check keeps any group that is not `trigger_disabled` (i.e., both `trigger_activated` **and** `trigger_pending`):
```typescript
if (groupStatus !== 'trigger_disabled') { // also includes TRIGGER_PENDING
```
This means a group whose conditions have not been met yet can lower (or raise) the effective minimum duration, preventing the buffer from flushing at the right time.
Consider restricting this to activated groups only:
```suggestion
if (groupStatus === 'trigger_activated') {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/browser/src/types.ts
Line: 517-523
Comment:
**Unused exported interface**
`SessionRecordingTriggerGroupsConfig` is exported but never referenced anywhere in the codebase. If it is not needed as part of the public API or future planning, it should be removed to keep the surface area clean (simplicity rule: no superfluous parts).
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(replay): account for V2 trigger gro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+413
to
+424
| for (const matcher of this._triggerGroupMatchers) { | ||
| const groupStatus = matcher.triggerStatus(this.sessionId) | ||
|
|
||
| // Only consider groups that are activated or pending | ||
| if (groupStatus !== 'trigger_disabled') { | ||
| const groupDuration = matcher.group.minDurationMs | ||
| if (isNumber(groupDuration)) { | ||
| if (isNull(lowestDuration) || groupDuration < lowestDuration) { | ||
| lowestDuration = groupDuration | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
_minimumDuration includes trigger-pending groups, not just activated ones
The PR description states: "Buffer flushes when duration meets the LOWEST minDurationMs among activated groups." However the current check keeps any group that is not trigger_disabled (i.e., both trigger_activated and trigger_pending):
if (groupStatus !== 'trigger_disabled') { // also includes TRIGGER_PENDINGThis means a group whose conditions have not been met yet can lower (or raise) the effective minimum duration, preventing the buffer from flushing at the right time.
Consider restricting this to activated groups only:
Suggested change
| for (const matcher of this._triggerGroupMatchers) { | |
| const groupStatus = matcher.triggerStatus(this.sessionId) | |
| // Only consider groups that are activated or pending | |
| if (groupStatus !== 'trigger_disabled') { | |
| const groupDuration = matcher.group.minDurationMs | |
| if (isNumber(groupDuration)) { | |
| if (isNull(lowestDuration) || groupDuration < lowestDuration) { | |
| lowestDuration = groupDuration | |
| } | |
| } | |
| } | |
| if (groupStatus === 'trigger_activated') { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Line: 413-424
Comment:
**`_minimumDuration` includes trigger-pending groups, not just activated ones**
The PR description states: *"Buffer flushes when duration meets the LOWEST `minDurationMs` among **activated** groups."* However the current check keeps any group that is not `trigger_disabled` (i.e., both `trigger_activated` **and** `trigger_pending`):
```typescript
if (groupStatus !== 'trigger_disabled') { // also includes TRIGGER_PENDING
```
This means a group whose conditions have not been met yet can lower (or raise) the effective minimum duration, preventing the buffer from flushing at the right time.
Consider restricting this to activated groups only:
```suggestion
if (groupStatus === 'trigger_activated') {
```
How can I resolve this? If you propose a fix, please make it concise.
packages/browser/src/types.ts
Outdated
Comment on lines
+517
to
+523
| /** | ||
| * V2 Trigger Groups configuration from remote config | ||
| */ | ||
| export interface SessionRecordingTriggerGroupsConfig { | ||
| version: 2 | ||
| groups: SessionRecordingTriggerGroup[] | ||
| } |
Contributor
There was a problem hiding this comment.
SessionRecordingTriggerGroupsConfig is exported but never referenced anywhere in the codebase. If it is not needed as part of the public API or future planning, it should be removed to keep the surface area clean (simplicity rule: no superfluous parts).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/types.ts
Line: 517-523
Comment:
**Unused exported interface**
`SessionRecordingTriggerGroupsConfig` is exported but never referenced anywhere in the codebase. If it is not needed as part of the public API or future planning, it should be removed to keep the surface area clean (simplicity rule: no superfluous parts).
How can I resolve this? If you propose a fix, please make it concise.
pauldambra
reviewed
Mar 26, 2026
Member
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
🤖 Bot review based on Paul D'Ambra's review patterns and c2 wiki XP principles. Two perspectives below — Paul's usual concerns (coupling, observability, safe rollouts) and an XP lens (simplicity rules, code smells). Comments are inline.
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Show resolved
Hide resolved
| eventTriggers: group.conditions.events, | ||
| linkedFlag: group.conditions.flag, | ||
| urlBlocklist: [], // groups don't have blocklist | ||
| } as any |
Member
There was a problem hiding this comment.
🤖 from paul's review patterns:
the as any cast here means if someone renames urlTriggers to urlPatterns or whatever, the compiler won't tell you this broke.
i wonder if it's worth making a narrow type that represents just the fields the individual matchers actually need? or even just as Pick<SessionRecordingRemoteConfig, 'urlTriggers' | 'eventTriggers' | ...>? that way the compiler is still in the loop.
not blocking — the tests would catch it — but i'm slightly allergic to as any in code that's this important to get right.
🤖 from c2 wiki / XP lens:
the deeper issue is that URLTriggerMatching, EventTriggerMatching, and LinkedFlagMatching each accept the full remote config bag and reach in for the 2 fields they care about — that's Feature Envy. if each matcher accepted just its own slice (EventTriggerMatching.onConfig({ eventTriggers: string[] })), TriggerGroupMatching could pass the right data without a cast. bigger change, not necessarily for this PR, but it's the direction that removes the as any properly.
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Show resolved
Hide resolved
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Outdated
Show resolved
Hide resolved
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Outdated
Show resolved
Hide resolved
| anyGroupActivated = true | ||
| // Check if this group's sample rate hit | ||
| const groupId = matcher.group.id | ||
| if (samplingResults.get(groupId) === true) { |
Member
There was a problem hiding this comment.
🤖 from c2 wiki / XP lens:
what's the intended meaning when a group is TRIGGER_ACTIVATED but samplingResults has no entry? the code returns ACTIVE, and the edge case test covers it, but the intent is ambiguous: does ACTIVE mean "we're recording but not because of sampling" or "something went wrong and we're recording as a fallback"?
if it truly shouldn't happen, a log warning would surface the bug. if it can happen (race between trigger activation and sampling decision), the behavior should be expressed in the function name or structure rather than relying on the comment.
packages/browser/src/types.ts
Outdated
| /** | ||
| * V2 Trigger Groups configuration from remote config | ||
| */ | ||
| export interface SessionRecordingTriggerGroupsConfig { |
Member
There was a problem hiding this comment.
🤖 from c2 wiki / XP lens:
SessionRecordingTriggerGroupsConfig is defined here but not imported or used anywhere. Rule 4 of Simple Design: no superfluous parts. If you need it later, you'll write it then and it'll match what you actually need. YAGNI says delete it.
| }) | ||
|
|
||
| describe('triggerGroupsMatchSessionRecordingStatus - Basic States', () => { | ||
| const createMockMatcher = ( |
Member
There was a problem hiding this comment.
🤖 from both paul's review patterns and c2 wiki:
createMockMatcher is defined three times across the describe blocks in this file with minor variations (one adds a sampleRate param). OnceAndOnlyOnce — a single version with optional overrides would deduplicate:
const createMockMatcher = (
id: string,
triggerStatus: TriggerStatus,
overrides?: Partial<{ sampleRate: number }>
): TriggerGroupMatching => ({
group: { id, name: `Group ${id}`, sampleRate: overrides?.sampleRate ?? 1.0, conditions: { matchType: 'any' } },
triggerStatus: () => triggerStatus,
stop: () => {},
} as unknown as TriggerGroupMatching)also — these tests are excellent candidates for parameterized tests (test.each). each test is: given these inputs, expect this status. that's a table, not a list of test functions 🚢
| if (isV2) { | ||
| // V2: Setup trigger group matchers | ||
| this._statusMatcher = triggerGroupsMatchSessionRecordingStatus | ||
| this._setupTriggerGroups(config.triggerGroups!) |
Member
There was a problem hiding this comment.
🤖 from c2 wiki / XP lens:
the V1/V2 branching in _onRemoteConfig, status, _minimumDuration, _flushBuffer, and URL trigger checking is a Shotgun Surgery smell — one concept (V2 trigger groups) is smeared across many methods.
ComposedMethod / Strategy pattern suggests: if every method asks "am I V1 or V2?", you likely have two strategies wanting to be separate objects. extracting V2 behavior into something like a TriggerGroupRecordingStrategy that owns the matchers, sampling results, flush flag, and min-duration logic would mean the V1/V2 branch exists in exactly one place. LazyLoadedSessionRecording is already a Large Class — every future V2 feature will add another if/else to every method.
not a "you must" — it works. but worth considering before V3 arrives 😄
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
9a6a3d1 to
6e763ad
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
packages/browser/src/__tests__/extensions/replay/triggerGroups-v1-compat.test.ts
Show resolved
Hide resolved
TueHaulund
reviewed
Mar 27, 2026
Contributor
TueHaulund
left a comment
TueHaulund
left a comment
There was a problem hiding this comment.
Had a couple of questions
| simpleEventEmitter.emit('eventCaptured', { event: 'test_event' }) | ||
|
|
||
| // Should be ACTIVE (matched but sampled out), not SAMPLED | ||
| expect(sessionRecording.status).toBe('active') |
Contributor
There was a problem hiding this comment.
Hmm, why is the session recording status ACTIVE if the session was not sampled?
packages/browser/src/extensions/replay/external/triggerMatching.ts
Outdated
Show resolved
Hide resolved
packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts
Show resolved
Hide resolved
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
6e763ad to
cb2e366
Compare
Comment on lines
200
to
202
| // _statusMatcher removed in favor of direct function calls in status getter | ||
| expect(sessionRecording['_lazyLoadedSessionRecording']['_triggerMatching']).toBeInstanceOf( | ||
| OrTriggerMatching |
There was a problem hiding this comment.
This test is checking for the removed _statusMatcher property. It should be updated to check the _strategy property type instead. Replace with: expect(sessionRecording['_lazyLoadedSessionRecording']['_strategy']).toBeInstanceOf(V1RecordingStrategy)
Spotted by Graphite (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
cb2e366 to
0b4dd3a
Compare
ksvat
force-pushed
the
account-for-new-trigger-groups
branch
from
0b4dd3a to
7e920c1
Compare
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
3 participants
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.
Problem
Introduce V2 trigger functionality to sdk
Changes
Implements V2 trigger groups for session recording with per-group sampling, per-group minimum duration checking, and per-group conditions.
$sdk_debug_replay_matched_recording_trigger_groups: [`` ``{ id: '1234', name: 'Error Tracking', matched: true, sampled: true },`` ``{ id: 'premium', name: 'Premium Users', matched: true, sampled: false }`` ``]tests
Backward Compatibility
V1 behavior is unchanged
Screen Recording 2026-03-24 at 9.33.26 PM.mov (uploaded via Graphite)
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file