Track session metrics accurately#437
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
💤 Files with no reviewable changes (11)
WalkthroughSessionTracker now resolves Clerk sign-in method (OAuth/email/clerk), caches current userId, counts per-session page views and interactions, emits user.signedIn on userId transition, reports accumulated duration/pageViews/interactions on beforeunload, and includes tests plus an analytics pageView param change. ChangesSession Tracking Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/SessionTracker.tsx (1)
113-150:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
loadTimeis measuring dwell time on the previous page, not the new route's load.Line 149 resets
pageLoadTimeRefafter the current page view is emitted. On the next pathname change, Line 113 subtracts that stale timestamp, so a user who spends five minutes on/listingswill report roughly five minutes ofloadTimefor/games. That writes inflated metrics intoanalytics.session.pageViewand undermines the accuracy goal of this PR. Capture a route-start timestamp when navigation begins, or stop sending this field for client-side navigations until that signal exists.🤖 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 `@src/components/SessionTracker.tsx` around lines 113 - 150, The current loadTime calculation uses pageLoadTimeRef.current which is reset after emitting a pageView, so on the next pathname change you end up reporting the previous page's dwell as the new page's loadTime; to fix this, capture a route-start timestamp when navigation begins (e.g., on your router's routeChangeStart or equivalent) and set pageLoadTimeRef.current there, then only compute and include loadTime in analytics.session.pageView when that route-start timestamp exists (otherwise omit or null the loadTime); update references in this component (pageLoadTimeRef, loadTime, analytics.session.pageView, pathname, the useEffect depending on [analyticsAllowed, pathname]) so you no longer reset pageLoadTimeRef.current only after emitting the pageView but instead set it at navigation start and reset/update it after the corresponding pageView is sent.
🧹 Nitpick comments (1)
src/components/SessionTracker.test.tsx (1)
100-100: 💤 Low valueConsider adding a comment to explain the pageView count.
The assertion that
pageViewis called once verifies that user sign-in alone does not trigger additional page views (only pathname changes do). A brief comment would clarify this subtle invariant.📝 Suggested comment
+ // pageView should still be 1 (user sign-in does not trigger pageView) expect(testState.analytics.session.pageView).toHaveBeenCalledOnce()🤖 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 `@src/components/SessionTracker.test.tsx` at line 100, Add a brief inline comment above the assertion in SessionTracker.test.tsx that explains why expect(testState.analytics.session.pageView).toHaveBeenCalledOnce() is asserted (i.e., to ensure that user sign-in does not create additional page views and only pathname changes should increment pageView); place the comment directly above the expect call so future readers understand the subtle invariant being tested.
🤖 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 `@src/components/SessionTracker.test.tsx`:
- Around line 44-123: The tests are missing coverage for the analytics-privacy
path where testState.analyticsAllowed is false; add a new spec in
SessionTracker.test.tsx that sets testState.analyticsAllowed = false, renders
<SessionTracker />, waits briefly (e.g., via setTimeout or waitFor) and then
asserts that testState.analytics.session.sessionStarted and
testState.analytics.session.pageView were not called; reference the existing
test patterns (render(<SessionTracker />),
testState.analytics.session.sessionStarted,
testState.analytics.session.pageView) to mirror style and placement alongside
the other it(...) blocks.
- Around line 80-122: Add a new test in SessionTracker.test.tsx that exercises
the third branch of getSignInMethod by rendering <SessionTracker />, waiting for
testState.analytics.session.sessionStarted, then setting testState.user = { id:
'user-3', primaryEmailAddress: null } (no externalAccounts) and rerendering the
component; finally assert that testState.analytics.user.signedIn was called with
{ method: 'clerk', userId: 'user-3' } to verify the clerk fallback.
- Around line 44-49: Add explicit mock clearing to ensure tests are isolated:
update the test setup for the SessionTracker suite to call Jest's mock
reset/clear (e.g., jest.clearAllMocks() or jest.resetAllMocks()) in the
beforeEach or in an afterEach so mock call counts are reset between tests; apply
this change near the existing beforeEach block that sets
testState.analyticsAllowed, testState.pathname and testState.user so assertions
like toHaveBeenCalledOnce() only see calls from the current test.
---
Outside diff comments:
In `@src/components/SessionTracker.tsx`:
- Around line 113-150: The current loadTime calculation uses
pageLoadTimeRef.current which is reset after emitting a pageView, so on the next
pathname change you end up reporting the previous page's dwell as the new page's
loadTime; to fix this, capture a route-start timestamp when navigation begins
(e.g., on your router's routeChangeStart or equivalent) and set
pageLoadTimeRef.current there, then only compute and include loadTime in
analytics.session.pageView when that route-start timestamp exists (otherwise
omit or null the loadTime); update references in this component
(pageLoadTimeRef, loadTime, analytics.session.pageView, pathname, the useEffect
depending on [analyticsAllowed, pathname]) so you no longer reset
pageLoadTimeRef.current only after emitting the pageView but instead set it at
navigation start and reset/update it after the corresponding pageView is sent.
---
Nitpick comments:
In `@src/components/SessionTracker.test.tsx`:
- Line 100: Add a brief inline comment above the assertion in
SessionTracker.test.tsx that explains why
expect(testState.analytics.session.pageView).toHaveBeenCalledOnce() is asserted
(i.e., to ensure that user sign-in does not create additional page views and
only pathname changes should increment pageView); place the comment directly
above the expect call so future readers understand the subtle invariant being
tested.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48437ec7-7d11-4083-ba7b-8d3cfb1948ee
📒 Files selected for processing (2)
src/components/SessionTracker.test.tsxsrc/components/SessionTracker.tsx
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.
Description
Tracks session metrics from actual client activity instead of fixed placeholders:
Fixes #392
Type of change
How Has This Been Tested?
Checks run:
./node_modules/.bin/vitest run src/components/SessionTracker.test.tsx./node_modules/.bin/eslint src/components/SessionTracker.tsx src/components/SessionTracker.test.tsx./node_modules/.bin/next typegen./node_modules/.bin/tsc --noEmitgit diff --checkScreenshots (if applicable)
N/A
Checklist
Notes for reviewers
The temp worktree reused the already-installed dependency tree and generated Prisma client from the main checkout for local verification, then ran Next type generation before TypeScript to match the repo's
typesscript order.Summary by CodeRabbit