Fix session recorder and even tracker access token fetch#1331
Merged
Conversation
The gate delayed flushes until an access token resolved, but sendBatch already resolves the session itself via _getSession() at send time, so _lastKnownAccessToken was a redundant readiness check that caused head-of-session events to be silently dropped on slow auth init and suppressed uploads entirely when token fetch failed.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughReworked analytics auth: EventTracker and SessionRecorder no longer accept or cache Changes
Sequence Diagram(s)sequenceDiagram
participant ClientApp as ClientApp
participant EventTracker as EventTracker
participant SessionRecorder as SessionRecorder
participant AnalyticsServer as AnalyticsServer
ClientApp->>EventTracker: construct(projectId, sendBatch)
ClientApp->>SessionRecorder: construct(projectId, sendBatch)
Note over EventTracker,SessionRecorder: buffer events locally
alt periodic tick when events exist
EventTracker->>EventTracker: _tick() -> _flush()
SessionRecorder->>SessionRecorder: _tick() -> _flush()
EventTracker->>ClientApp: request getAnalyticsSession()
SessionRecorder->>ClientApp: request getAnalyticsSession()
ClientApp->>EventTracker: return analytics session
ClientApp->>SessionRecorder: return analytics session
EventTracker->>AnalyticsServer: sendAnalyticsEventBatch(events, session)
SessionRecorder->>AnalyticsServer: sendSessionReplayBatch(events, session)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR removes the Confidence Score: 5/5Safe to merge — only a P2 style finding remains. The auth-gate removal is well-reasoned and the code path is sound. The single finding is a minor inconsistency where packages/template/src/lib/stack-app/apps/implementations/session-replay.ts — the Important Files Changed
Sequence DiagramsequenceDiagram
participant Timer as setInterval (_tick)
participant Recorder as SessionRecorder
participant SendBatch as sendBatch closure
participant Interface as _interface.sendSessionReplayBatch
Note over Timer,Interface: Before (auth-gated)
Timer->>Recorder: _tick()
Recorder->>Recorder: refresh _lastKnownAccessToken (async)
alt _lastKnownAccessToken is null
Recorder-->>Timer: skip flush (events dropped)
else token present
Recorder->>SendBatch: _flush()
SendBatch->>Interface: sendSessionReplayBatch(body, session, opts)
end
Note over Timer,Interface: After (auth removed)
Timer->>Recorder: _tick()
Recorder->>SendBatch: _flush() (if events queued)
SendBatch->>Interface: sendSessionReplayBatch(body, await _getSession(), opts)
Interface-->>SendBatch: Result
SendBatch-->>Recorder: Result
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/template/src/lib/stack-app/apps/implementations/session-replay.ts
Line: 325
Comment:
**Inconsistent `noErrorLogging` option removed in `_tick`**
Every other call to `runAsynchronously(() => this._flush(...))` in this file keeps `{ noErrorLogging: true }` — `stop()`, the batch-size threshold in `_startRecording`, and the `pagehide`/`visibilitychange` listener all do. Dropping it here means any unhandled exception from `_flush` (e.g., if `sendBatch`'s inner `await this._getSession()` rejects) will be forwarded to `captureError` on every 5-second tick, potentially flooding error-tracking dashboards for transient auth failures.
```suggestion
runAsynchronously(() => this._flush({ keepalive: false }), { noErrorLogging: true });
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Remove auth gate from session replay rec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/template/src/lib/stack-app/apps/implementations/session-replay.ts (1)
322-327: Add{ noErrorLogging: true }for consistency.All other
runAsynchronouslycalls in this file (lines 171, 184, 287, 298) include{ noErrorLogging: true }, but this one does not. Since_flushalready logs warnings internally when errors occur, omitting the option here may cause redundant error output.♻️ Suggested fix
private _tick() { if (this._cancelled) return; if (this._events.length > 0) { - runAsynchronously(() => this._flush({ keepalive: false })); + runAsynchronously(() => this._flush({ keepalive: false }), { noErrorLogging: true }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/template/src/lib/stack-app/apps/implementations/session-replay.ts` around lines 322 - 327, The runAsynchronously call inside the private method _tick currently omits the options object and should pass { noErrorLogging: true } for consistency; update the invocation of runAsynchronously(() => this._flush({ keepalive: false })) in _tick to include the options parameter so it becomes runAsynchronously(() => this._flush({ keepalive: false }), { noErrorLogging: true }) to avoid duplicate error output (referencing the _tick method, runAsynchronously helper, and _flush function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/template/src/lib/stack-app/apps/implementations/session-replay.ts`:
- Around line 322-327: The runAsynchronously call inside the private method
_tick currently omits the options object and should pass { noErrorLogging: true
} for consistency; update the invocation of runAsynchronously(() =>
this._flush({ keepalive: false })) in _tick to include the options parameter so
it becomes runAsynchronously(() => this._flush({ keepalive: false }), {
noErrorLogging: true }) to avoid duplicate error output (referencing the _tick
method, runAsynchronously helper, and _flush function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e984cf6-d22d-4fba-93f7-36df88bada31
📒 Files selected for processing (2)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/implementations/session-replay.ts
💤 Files with no reviewable changes (1)
- packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Same pattern as SessionRecorder: sendBatch already resolves the session via _getSession() at send time, so the _lastKnownAccessToken readiness check was redundant and delayed the first batch by an extra tick. getAnalyticsAccessToken is now unused and removed from the construction site.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/template/src/lib/stack-app/common.ts`:
- Around line 53-56: Add a temporary deprecated alias "anonymous" to the
GetCurrentPartialUserOptions union so existing callers of getPartialUser({ or:
"anonymous" }) continue to compile: update the union to include 'anonymous'
alongside 'anonymous-if-exists', add a comment "// TODO next-release" next to
the alias, and edit the existing inline note to reference the canonical
"anonymous-if-exists" value; also mirror this compatibility shim inside the
_getTokenPartialUserFromSession(...) implementation so it accepts "anonymous" as
equivalent to "anonymous-if-exists" until the next release.
🪄 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: a4cfb03e-3c15-471f-ac75-f04624e873d1
📒 Files selected for processing (4)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/implementations/event-tracker.test.tspackages/template/src/lib/stack-app/apps/implementations/event-tracker.tspackages/template/src/lib/stack-app/common.ts
Use getPartialUser to detect a cached token; fall back to
getUser({ or: 'anonymous' }) so anonymous sign-up happens in the analytics
path itself rather than relying on a separate access-token gate. Also drop
the `noErrorLogging: true` flags from the _flush calls so unexpected
failures surface instead of being swallowed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 641f7320-9a8b-4a36-8f20-85b21f0e50cb
📒 Files selected for processing (4)
examples/demo/src/app/token-staleness/page.tsxpackages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/apps/implementations/event-tracker.tspackages/template/src/lib/stack-app/apps/implementations/session-replay.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts
BilalG1
changed the title
N2D4
approved these changes
Apr 14, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (1)
556-580:⚠️ Potential issue | 🟠 MajorDon't move anonymous sign-up into the unload flush path.
This still resolves
getUser({ or: "anonymous" })on the send path. If the replay/event unload handlers only make the final batch uploadkeepalive, first-visit anonymous users can still lose the whole batch because the prerequisite user fetch/sign-up is a normal request. Warm the analytics session when recording starts or when the first item is buffered so unload only needs the keepalive upload.Please verify the unload/pagehide flow in the adjacent files:
#!/bin/bash set -euo pipefail echo "== analytics session lookup ==" rg -n -C3 'getAnalyticsSession|sendSessionReplayBatch|sendAnalyticsEventBatch' \ packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts for f in \ packages/template/src/lib/stack-app/apps/implementations/session-replay.ts \ packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts do echo echo "== $f ==" rg -n -C4 'pagehide|beforeunload|visibilitychange|keepalive|sendBatch' "$f" doneExpected result: unload/pagehide should not require any pre-upload request before the keepalive batch send.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts` around lines 556 - 580, The unload flow still calls getUser({or: "anonymous"}) via getAnalyticsSession() during sendBatch, causing a non-keepalive request; to fix, eagerly warm and cache the analytics session when recording starts (or on first buffered item) so unload handlers only use the cached session. Specifically, in client-app-impl.ts ensure getAnalyticsSession() is invoked and its result stored before calling this._sessionRecorder.start() and before creating/starting this._eventTracker (or hook into SessionRecorder/EventTracker to call and cache getAnalyticsSession() when the first event/replay is buffered), then have the sendBatch closures use the cached session value instead of calling getAnalyticsSession() each time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 2693-2695: The check that returns null for anonymous tokens should
accept both the new and deprecated literal; update the conditional around
isAnonymous and options.or to treat "anonymous-if-exists" OR
"anonymous-if-exists[deprecated]" as equivalent (and add a comment // TODO
next-release) and also add the same temporary alias to the
GetCurrentPartialUserOptions type/enum so the old literal remains supported for
one release.
---
Duplicate comments:
In `@packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts`:
- Around line 556-580: The unload flow still calls getUser({or: "anonymous"})
via getAnalyticsSession() during sendBatch, causing a non-keepalive request; to
fix, eagerly warm and cache the analytics session when recording starts (or on
first buffered item) so unload handlers only use the cached session.
Specifically, in client-app-impl.ts ensure getAnalyticsSession() is invoked and
its result stored before calling this._sessionRecorder.start() and before
creating/starting this._eventTracker (or hook into SessionRecorder/EventTracker
to call and cache getAnalyticsSession() when the first event/replay is
buffered), then have the sendBatch closures use the cached session value instead
of calling getAnalyticsSession() each time.
🪄 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: a8da992a-68a4-4da1-b5b4-267e60e4d77e
📒 Files selected for processing (2)
packages/template/src/lib/stack-app/apps/implementations/client-app-impl.tspackages/template/src/lib/stack-app/common.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/template/src/lib/stack-app/common.ts
BilalG1
added a commit
that referenced
this pull request
Apr 14, 2026
Owned admin apps are constructed with `tokenStore: null`, which caused EventTracker/SessionRecorder flushes to throw from _ensurePersistentTokenStore() after #1331 removed the silencing.
BilalG1
added a commit
that referenced
this pull request
Apr 14, 2026
Owned admin apps are constructed with `tokenStore: null`, which caused EventTracker/SessionRecorder flushes to throw from _ensurePersistentTokenStore() after #1331 removed the silencing. <!-- Make sure you've read the CONTRIBUTING.md guidelines: https://github.com/stack-auth/stack-auth/blob/dev/CONTRIBUTING.md --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved analytics stability and privacy by restricting session recording and event tracking to environments with required persistent storage. * **Tests** * Adjusted a few end-to-end tests to skip when running against a local emulator to reduce spurious failures. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
2 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.
The gate delayed flushes until an access token resolved, but sendBatch already resolves the session itself via _getSession() at send time, so _lastKnownAccessToken was a redundant readiness check that caused head-of-session events to be silently dropped on slow auth init and suppressed uploads entirely when token fetch failed.
Summary by CodeRabbit