packages/stack-shared/src/interface/client-interface.ts (1)

253-298: Both batch methods use a catch-all that swallows KnownError type information.

sendClientRequest can throw typed KnownError instances (e.g., AnalyticsNotEnabled). The catch-all on lines 272–274 / 296–297 wraps them as plain Error in Result.error, so callers lose the ability to distinguish specific failure reasons. This is fine if callers only need success/failure, but it deviates from the project guideline against try-catch-all.

If the caller should be able to react to AnalyticsNotEnabled specifically (e.g., to stop sending further batches), consider using sendClientRequestAndCatchKnownError instead, or at least preserving the original error object rather than re-wrapping it as new Error(String(e)).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-shared/src/interface/client-interface.ts` around lines 253 -
298, The catch-all in sendSessionReplayBatch and sendAnalyticsEventBatch hides
typed KnownError (e.g., AnalyticsNotEnabled) by wrapping exceptions into new
Error, so update these methods to preserve KnownError information: either call
sendClientRequestAndCatchKnownError instead of sendClientRequest (so KnownError
is handled explicitly) or, if keeping sendClientRequest, return Result.error(e
instanceof Error ? e : new Error(String(e))) without re-wrapping KnownError
(i.e., don't create new Error for KnownError instances) — ensure the catch
distinguishes KnownError and returns it directly to Result.error while only
normalizing unknowns, referencing sendSessionReplayBatch,
sendAnalyticsEventBatch, sendClientRequest, sendClientRequestAndCatchKnownError,
KnownError, and AnalyticsNotEnabled.

packages/template/src/lib/stack-app/apps/implementations/event-tracker.ts (2)

261-278: getAccessToken result is asynchronous but hasAuth is read synchronously — one-tick lag on auth state changes.

_tick fires getAccessToken via runAsynchronously (fire-and-forget) and then immediately reads this._lastKnownAccessToken synchronously. This means the auth state always reflects the previous tick's result. During user transitions (logout → login as a different user), there's a ~10s window where:

1. Events from the new user could be flushed with the old user's stale token (server rejects).
2. On the next tick, _wasAuthenticated && !hasAuth clears the buffer, possibly discarding valid events from the new user.

For an internal-only analytics tracker this is acceptable, but worth documenting with a brief comment.

🤖 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/event-tracker.ts`
around lines 261 - 278, The _tick method fires getAccessToken asynchronously and
then immediately reads this._lastKnownAccessToken, causing a one-tick stale auth
state; change _tick so it updates and uses the token result before making
decisions: call this._deps.getAccessToken() and await its result (or chain a
.then) inside _tick to set this._lastKnownAccessToken and compute hasAuth from
that fresh value, then perform the logout buffer clear and conditional flush
using the fresh hasAuth; update references to _wasAuthenticated, _events,
_approxBytes and _flush accordingly so they act on the current token state
rather than the previous tick.

---

68-80: Events lost silently on flush failure.

In _flush (line 243–244), the event buffer is cleared before the network call completes. If sendBatch fails, those events are permanently discarded with only a console.warn. This is a standard trade-off in analytics SDKs (no guaranteed delivery), but it's worth noting as a conscious design choice — especially since noErrorLogging: true on the runAsynchronously calls means these failures won't surface through the normal error pipeline either.

🤖 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/event-tracker.ts`
around lines 68 - 80, The code currently clears the event buffer inside _flush
before calling sendBatch which causes silent loss if sendBatch fails; modify
_flush so it does not mutate or drop this._events/_approxBytes until sendBatch
succeeds (or at least re-queues them on failure): capture the current batch
(e.g., const batch = this._events.slice() and bytes copy) and set this._events
and this._approxBytes to a fresh buffer only after sendBatch resolves, otherwise
restore or prepend the failed batch back into this._events and adjust
_approxBytes; also remove or reconsider using noErrorLogging for the
runAsynchronously calls around _flush so failures surface via the normal error
pipeline, referencing _pushEvent, _flush, sendBatch, and runAsynchronously to
locate the changes.

apps/e2e/tests/js/cookies.test.ts (1)

7-23: Consider making pathname configurable in BrowserEnvOptions

pathname is now part of BrowserEnv but there is no corresponding option in BrowserEnvOptions, so all tests that call setupBrowserCookieEnv are locked to "/". Since the new EventTracker uses window.location.pathname for page-view events, future tests that need to assert page-view tracking on non-root paths will need to mutate location.pathname manually after setup — a pattern that can be fragile.

🔧 Proposed refactor

 type BrowserEnvOptions = {
   host?: string,
   protocol?: "https:" | "http:",
+  pathname?: string,
 };

   const {
     host = "app.example.com",
     protocol = "https:",
+    pathname = "/",
   } = options;

     pathname: "/",

-    pathname: "/",
+    pathname,

Also applies to: 25-50

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/e2e/tests/js/cookies.test.ts` around lines 7 - 23, Add a pathname option
to the test environment options and thread it into the environment setup: update
the BrowserEnvOptions type to include pathname?: string, default to "/" when not
provided in setupBrowserCookieEnv, and use that value when building the
BrowserEnv.location.pathname (and any derived href/origin if needed). Modify
setupBrowserCookieEnv to accept and apply the pathname option so tests can
initialize non-root window.location.pathname without manual mutation; keep
BrowserEnv and existing callers working by falling back to "/".

apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx (1)

1357-1365: clickCountsByReplayId.get(r.id) is called twice; extract to a variable.

♻️ Proposed refactor

+                          const clickCount = clickCountsByReplayId.get(r.id) ?? 0;
                           <div className="flex items-center justify-between gap-2 text-xs text-muted-foreground">
                             <DisplayDate date={r.lastEventAt} />
-                            {(clickCountsByReplayId.get(r.id) ?? 0) > 0 && (
+                            {clickCount > 0 && (
                               <span className="flex items-center gap-0.5 text-[10px] text-muted-foreground/70">
                                 <CursorClickIcon className="h-3 w-3" />
-                                {clickCountsByReplayId.get(r.id)}
+                                {clickCount}
                               </span>
                             )}
                           </div>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/analytics/replays/page-client.tsx
around lines 1357 - 1365, The code calls clickCountsByReplayId.get(r.id) twice;
extract it to a local variable (e.g., const clickCount =
clickCountsByReplayId.get(r.id) ?? 0) before the JSX and use clickCount in both
the conditional and the rendered value so the map lookup is performed once;
update the JSX that references clickCountsByReplayId.get(r.id) and the
conditional to use clickCount, keeping DisplayDate and CursorClickIcon
unchanged.

packages/template/src/lib/stack-app/apps/implementations/session-replay.ts (1)

120-132: Exported getOrRotateSession accesses localStorage without a browser guard

While getOrRotateSession is exported and calls localStorage directly (line 121), all current internal call sites are protected by the isBrowserLike() guard in SessionRecorder.start() (line 173). The function is also not re-exported from the public API (index.ts), and no external imports exist in the codebase.

However, since this is an exported function, adding a defensive guard follows best practices:

🛡️ Proposed fix

 export function getOrRotateSession(options: { key: string, nowMs: number }): StoredSession {
+  if (typeof localStorage === "undefined") throwErr("getOrRotateSession called outside a browser context");
   const existing = safeParseStoredSession(localStorage.getItem(options.key));

🤖 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 120 - 132, getOrRotateSession currently accesses localStorage
directly which can throw in non-browser environments; update getOrRotateSession
to guard access to localStorage (e.g., using the existing isBrowserLike check)
and handle non-browser cases by returning a sensible fallback or throwing a
clear error. Specifically, inside getOrRotateSession (and before calling
safeParseStoredSession or localStorage.setItem) verify isBrowserLike() or
equivalent, and if false return a new StoredSession object (or reject the
rotation) so callers such as SessionRecorder.start remain safe when this
exported function is used outside the browser.

apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/analytics/replays/session-replay-machine.test.ts (1)

559-587: LGTM — new tests correctly validate pausedAtGlobalMs = currentGlobalTimeMsForUi for all three pause-from modes.

All three scenarios (playing, gap_fast_forward, buffering) are covered with explicit currentGlobalTimeMsForUi overrides, making the expected source of pausedAtGlobalMs unambiguous.

Optional: the three new .toBe() assertions on lines 563, 571, and 584 could be expressed as .toMatchInlineSnapshot() per project test guidelines.

-      expect(s.pausedAtGlobalMs).toBe(2500);
+      expect(s.pausedAtGlobalMs).toMatchInlineSnapshot(`2500`);

-      expect(s.pausedAtGlobalMs).toBe(3000);
+      expect(s.pausedAtGlobalMs).toMatchInlineSnapshot(`3000`);

-      expect(s.pausedAtGlobalMs).toBe(1500);
+      expect(s.pausedAtGlobalMs).toMatchInlineSnapshot(`1500`);

Based on learnings: "Prefer .toMatchInlineSnapshot over other selectors in tests, and check/modify snapshot-serializer.ts for snapshot formatting."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/analytics/replays/session-replay-machine.test.ts
around lines 559 - 587, Replace the three explicit expect(...).toBe(...)
assertions that check pausedAtGlobalMs in the tests "pauses from playing",
"pauses from gap_fast_forward", and "pauses from buffering" with
expect(...).toMatchInlineSnapshot() calls per project snapshot style; ensure the
snapshot values reflect currentGlobalTimeMsForUi for each case and run the test
suite to update generated inline snapshots, and if snapshot formatting differs,
adjust snapshot-serializer.ts so the inline snapshot output matches project
conventions.