Conversation
Closed
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRestructures tracking code into a plugins surface, adds session-scoring and link-click plugins and tests, extracts cookie-based Storage and a UID storage abstraction, re-exports plugins from Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser/App
participant Session as enableSessionScoring
participant Storage as Storage (localStorage / CookieStorage)
participant BotDetect as getBotSignals
participant Plausible as Plausible SDK
participant Network as Network (POST /api/event)
Browser->>Session: enableSessionScoring(plausible, config)
alt running in browser
Session->>Storage: read/initialize firstVisit
Storage-->>Session: firstVisit timestamp
Session->>Session: requestAnimationFrame (next frame)
Session->>BotDetect: getBotSignals()
BotDetect->>BotDetect: evaluate navigator, UA, plugins, languages, permissions, hw
BotDetect-->>Session: {isBot, score, signals}
Session->>Plausible: event("Session scored", {botScore, botSignals, sessionAge, ...})
Plausible->>Network: POST /api/event (keepalive: true)
Network-->>Plausible: 200 OK
else server-side
Session-->>Browser: noop
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/enableAutoOutboundTracking.test.ts`:
- Around line 25-80: The tests use internal fragment links but expect "Outbound
Link: Click" events; update the fixtures and assertions so positive cases use
real external URLs (e.g., replace href="#foo" / "#baz" with
"https://external.example/..." in document.body and adapt expected p.url
accordingly) and keep captureText behavior for link text (span click). Also add
a negative test case that clicks a same-page/hash link (e.g., href="#bar") and
asserts mockFetch was not called (or call count unchanged) to ensure
enableAutoOutboundTracking does not report internal fragment links; reference
the test names ("Capture click for simple link", "Capture click for link text")
and the enableAutoOutboundTracking import to locate where to change fixtures and
expectations.
In `@src/plugins/enableAutoOutboundTracking.ts`:
- Around line 31-42: Before sending the outbound event, parse the clicked href
with new URL(linkElement.href, document.baseURI) and skip sending if the link is
internal: return early when urlObj.origin === location.origin (same-origin) and
also return when the link only changes the hash on the current page
(urlObj.pathname === location.pathname && urlObj.search === location.search but
urlObj.hash !== location.hash). Keep the existing filter(url, text) check
afterward and only call plausible.sendEvent(eventName, { props: { url, text:
captureText ? text : undefined } }) for external/outbound links.
- Around line 26-32: The find() callback needs an explicit type predicate so the
returned linkElement is recognized as an HTMLAnchorElement and textContent must
be null-guarded before calling trim(); update the call to
event.composedPath().find(...) to use a predicate like function isAnchor(node):
node is HTMLAnchorElement { return node instanceof HTMLAnchorElement } (or
inline equivalent) so linkElement is typed as HTMLAnchorElement, then get text
with something like (linkElement.textContent ?? '').trim() (leave url =
linkElement.href as-is).
In `@src/plugins/enableSessionScoring.test.ts`:
- Around line 24-38: The test currently snapshots the full machine-dependent
scoring payload (via mockFetch.mock.calls) which makes it flaky; instead update
the test for enableSessionScoring to avoid a full snapshot: keep triggering the
scoring (create Plausible, call enableSessionScoring,
vi.advanceTimersToNextFrame), then assert that mockFetch was called (e.g.,
mockFetch.mock.calls.length > 0) and inspect the request body of the first call
to assert only invariant fields (for example check that the payload contains
expected event_name, domain, and a numeric/boolean shape for scoring fields)
while explicitly excluding or asserting patterns for runtime-dependent fields
(timezone, hardwareConcurrency, screen dimensions) rather than snapshotting the
entire body; reference mockFetch.mock.calls and the enableSessionScoring
invocation to locate where to change assertions.
In `@src/plugins/enableSessionScoring.ts`:
- Around line 71-74: The enableSessionScoring export currently accepts only
plausible and a config with storage/firstVisitKey, but the API must support an
optional callback to add custom features to the scoring event; update the
enableSessionScoring signature to accept a new optional callback (e.g.,
customScoringHook or augmentScoringEvent) in the config object, thread that
callback through wherever the scoring event is built/sent so it can append/merge
extra properties, and update any related functions (the scoring event
creator/sender) to call this callback with the current session/context and merge
its return into the final event payload; also add/update tests to pass a mock
callback and assert its returned props are included in the emitted scoring
event.
- Around line 77-82: The storage selection and usages must be made fail-safe:
wrap the runtime resolution of storage (the local variable storage initialized
with localStorage/sessionStorage/CookieStorage and firstVisitKey) in a try/catch
so any exception accessing localStorage/sessionStorage falls back to new
CookieStorage(), and replace direct storage.getItem/setItem/removeItem calls
with a small safe wrapper (e.g., safeStorage.getItem/setItem/removeItem) that
catches and swallows quota/privacy errors (no-ops on failure) to make
reads/writes best-effort; update code paths that reference storage (the initial
resolver and subsequent usages around the firstVisitKey logic) to use this safe
wrapper.
- Around line 44-58: getBotSignals currently mutates score/signals inside
permission.query.then which runs after getBotSignals returns, so
permission_mismatch is dropped; change getBotSignals to be async and await the
permission.query() result (handle promise rejection with try/catch) before
computing/returning isBot and the payload, and apply the same async/await fix to
the duplicate block around lines 95-106; specifically locate getBotSignals, the
permission.query(...) usage, and the places that compute isBot/emit the payload
and ensure the permission.query path updates score and signals synchronously
before returning.
- Around line 113-115: The payload building code unconditionally calls
navigator.languages.join(',') which can throw in browsers without
navigator.languages; update the payload logic (where timeZone, language,
languages are set — the same block that calls getBotSignals()) to guard access
to navigator.languages before calling .join: check that navigator.languages is
an array (or truthy) and only then call join(',') otherwise provide a safe
fallback (e.g., empty string or the existing no_languages marker) so session
scoring doesn't throw in locked-down browsers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6f624f8-f408-4be0-88b6-2343b214ddf5
⛔ Files ignored due to path filters (1)
src/plugins/__snapshots__/enableSessionScoring.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
.eslintrcsrc/Plausible.server-environment.test.tssrc/enableAutoOutboundTracking.tssrc/index.tssrc/plugins/enableAutoOutboundTracking.test.tssrc/plugins/enableAutoOutboundTracking.tssrc/plugins/enableAutoPageviews.tssrc/plugins/enableSessionScoring.test.tssrc/plugins/enableSessionScoring.tssrc/plugins/index.tssrc/transformers/CookieStorage.tssrc/transformers/userId.test.tssrc/transformers/userId.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- src/enableAutoOutboundTracking.ts
src/plugins/enableSessionScoring.ts
Outdated
Comment on lines
+44
to
+58
| // 6. Permissions API anomaly (common headless trick) | ||
| try { | ||
| const permission = (navigator as any).permissions; | ||
| if (permission) { | ||
| permission.query({ name: 'notifications' }).then((result: any) => { | ||
| if (Notification.permission === 'denied' && result.state === 'prompt') { | ||
| score += 1; | ||
| signals.push('permission_mismatch'); | ||
| } | ||
| }); | ||
| } | ||
| } catch { | ||
| score += 1; | ||
| signals.push('permissions_error'); | ||
| } |
There was a problem hiding this comment.
getBotSignals() drops the permissions signal.
permission.query(...).then(...) mutates score after getBotSignals() has already returned and after isBot is computed, so permission_mismatch is never reliably reflected in the emitted event. Make bot detection async and await it before building the payload.
Proposed fix
-export function getBotSignals(): BotDetectionResult {
+export async function getBotSignals(): Promise<BotDetectionResult> {
let score = 0;
const signals: string[] = [];
@@
try {
const permission = (navigator as any).permissions;
if (permission) {
- permission.query({ name: 'notifications' }).then((result: any) => {
- if (Notification.permission === 'denied' && result.state === 'prompt') {
- score += 1;
- signals.push('permission_mismatch');
- }
- });
+ const result = await permission.query({
+ name: 'notifications' as PermissionName,
+ });
+ if (Notification.permission === 'denied' && result.state === 'prompt') {
+ score += 1;
+ signals.push('permission_mismatch');
+ }
}
} catch {
score += 1;
@@
- const botSignals = getBotSignals();
- requestAnimationFrame(() => {
+ requestAnimationFrame(async () => {
+ const botSignals = await getBotSignals();
const firstVisit = getFirstVisit();Also applies to: 95-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/enableSessionScoring.ts` around lines 44 - 58, getBotSignals
currently mutates score/signals inside permission.query.then which runs after
getBotSignals returns, so permission_mismatch is dropped; change getBotSignals
to be async and await the permission.query() result (handle promise rejection
with try/catch) before computing/returning isBot and the payload, and apply the
same async/await fix to the duplicate block around lines 95-106; specifically
locate getBotSignals, the permission.query(...) usage, and the places that
compute isBot/emit the payload and ensure the permission.query path updates
score and signals synchronously before returning.
Comment on lines
+71
to
+74
| export const enableSessionScoring = ( | ||
| plausible: Plausible, | ||
| config: { storage?: Storage; firstVisitKey?: string } = {}, | ||
| ) => { |
There was a problem hiding this comment.
The custom scoring hook from the PR objective is still missing.
The linked requirement calls for an optional callback to append custom features to the scoring event, but this API only accepts storage and firstVisitKey. As written, consumers cannot add their own session-scoring props.
I can help sketch the callback shape and the matching tests if you want to add it in this PR.
Also applies to: 102-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugins/enableSessionScoring.ts` around lines 71 - 74, The
enableSessionScoring export currently accepts only plausible and a config with
storage/firstVisitKey, but the API must support an optional callback to add
custom features to the scoring event; update the enableSessionScoring signature
to accept a new optional callback (e.g., customScoringHook or
augmentScoringEvent) in the config object, thread that callback through wherever
the scoring event is built/sent so it can append/merge extra properties, and
update any related functions (the scoring event creator/sender) to call this
callback with the current session/context and merge its return into the final
event payload; also add/update tests to pass a mock callback and assert its
returned props are included in the emitted scoring event.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/plugins/enableSessionScoring.test.ts (1)
36-48:⚠️ Potential issue | 🟠 MajorAvoid exact assertions for runtime-dependent scoring fields.
This test is still flaky because it hard-codes machine-specific values like
hardwareConcurrency, and it also dereferencesmockFetch.mock.calls[0]before proving the request was sent. Assert the call first, then pin only invariant fields and use flexible matchers for environment-dependent ones.Suggested change
- const eventBody = JSON.parse(mockFetch.mock.calls[0][1].body); + expect(mockFetch).toHaveBeenCalled(); + const eventBody = JSON.parse(String(mockFetch.mock.calls[0][1].body)); const eventName = eventBody.n; const payload = JSON.parse(eventBody.p); expect(payload).toMatchObject({ botScore: 1, botSignals: 'no_plugins', sessionAge: 0, language: 'en-US', languages: 'en-US,en', - screenSize: '0x0', - hardwareConcurrency: 16, - deviceMemory: 0, - devicePixelRatio: 1, + screenSize: expect.stringMatching(/^\d+x\d+$/), + hardwareConcurrency: expect.any(Number), + deviceMemory: expect.any(Number), + devicePixelRatio: expect.any(Number), }); expect(eventName).toBe('Session scored');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/enableSessionScoring.test.ts` around lines 36 - 48, The test is brittle because it dereferences mockFetch.mock.calls[0] before asserting a call and hard-codes runtime-dependent values; first assert mockFetch was called (e.g., expect(mockFetch).toHaveBeenCalled()), then parse the first call's body (eventBody -> payload) and assert only invariant fields via toMatchObject (botScore, botSignals, sessionAge, language) while using flexible Jest matchers for environment-dependent fields (e.g., expect.any(Number) or expect.stringMatching(...) for hardwareConcurrency, deviceMemory, devicePixelRatio, screenSize, and languages) so the test no longer relies on machine-specific exact values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/plugins/enableSessionScoring.test.ts`:
- Around line 36-48: The test is brittle because it dereferences
mockFetch.mock.calls[0] before asserting a call and hard-codes runtime-dependent
values; first assert mockFetch was called (e.g.,
expect(mockFetch).toHaveBeenCalled()), then parse the first call's body
(eventBody -> payload) and assert only invariant fields via toMatchObject
(botScore, botSignals, sessionAge, language) while using flexible Jest matchers
for environment-dependent fields (e.g., expect.any(Number) or
expect.stringMatching(...) for hardwareConcurrency, deviceMemory,
devicePixelRatio, screenSize, and languages) so the test no longer relies on
machine-specific exact values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32ef5699-db0e-4424-8f0b-d688a793bf87
📒 Files selected for processing (1)
src/plugins/enableSessionScoring.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/plugins/enableAutoOutboundTracking.test.ts (1)
21-26: Move plugin activation inside tests orbeforeEachfor proper isolation.
enableAutoOutboundTrackingis called once at module scope, which means the click listener is never cleaned up between tests. If a test modifies state or if the listener accumulates side effects, subsequent tests may see unexpected behavior.Proposed fix
const plausible = new Plausible({ apiHost: 'https://plausible.io', domain: 'example.org', }); -enableAutoOutboundTracking(plausible, { captureText: true }); +let cleanup: () => void; + +beforeEach(() => { + // ... existing beforeEach code ... + cleanup = enableAutoOutboundTracking(plausible, { captureText: true }); +}); + +afterEach(() => { + cleanup(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugins/enableAutoOutboundTracking.test.ts` around lines 21 - 26, The plugin activation is done at module scope (enableAutoOutboundTracking(plausible, ...)) which prevents cleanup between tests; move the call into each test or a beforeEach block so each test gets a fresh setup, and ensure you remove the listener in afterEach (either by calling the teardown/disposer returned by enableAutoOutboundTracking if it returns one, or by calling the appropriate Plausible instance cleanup method or removing the event listener manually) to avoid cross-test leakage; update tests that reference the module-scope plausible instance to create a new Plausible() per test or in beforeEach and store any disposer in a variable so afterEach can call it to restore isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugins/enableAutoOutboundTracking.ts`:
- Around line 14-18: The filter(url, text) function incorrectly uses
url.startsWith(location.origin) which can misclassify external look-alike
domains; replace the prefix check with robust origin comparison by constructing
a URL object for the candidate (handling relative URLs by using location as base
when in browser) and compare its origin to location.origin (or use try/catch to
safely handle invalid URLs). Update the logic in filter to only return false
when new URL(url, location.href).origin === location.origin, and ensure the
check is guarded by the same typeof window !== 'undefined' condition and handles
URL construction errors gracefully.
In `@src/plugins/enableLinkClicksCapture.ts`:
- Around line 28-34: The code uses event.composedPath().find(...) which
TypeScript doesn't narrow to HTMLAnchorElement and then calls
linkElement.textContent.trim() which can be null; update the find call to use a
type predicate (e.g., (node): node is HTMLAnchorElement => node instanceof
HTMLAnchorElement) so linkElement is properly typed, and guard
linkElement.textContent before trimming (either return early if null or use a
safe fallback like an empty string) so calling .trim() cannot throw; refer to
linkElement, event.composedPath(), and textContent when making the changes.
---
Nitpick comments:
In `@src/plugins/enableAutoOutboundTracking.test.ts`:
- Around line 21-26: The plugin activation is done at module scope
(enableAutoOutboundTracking(plausible, ...)) which prevents cleanup between
tests; move the call into each test or a beforeEach block so each test gets a
fresh setup, and ensure you remove the listener in afterEach (either by calling
the teardown/disposer returned by enableAutoOutboundTracking if it returns one,
or by calling the appropriate Plausible instance cleanup method or removing the
event listener manually) to avoid cross-test leakage; update tests that
reference the module-scope plausible instance to create a new Plausible() per
test or in beforeEach and store any disposer in a variable so afterEach can call
it to restore isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b1c8c25-0cc4-45a8-8fcf-0c4b8456fae5
📒 Files selected for processing (4)
src/plugins/enableAutoOutboundTracking.test.tssrc/plugins/enableAutoOutboundTracking.tssrc/plugins/enableLinkClicksCapture.tssrc/plugins/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/index.ts
Owner
Author
|
@CodeRabbit generate patch to update readme according to changes from most recent release to current commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 14762
--- a/README.md
+++ b/README.md
@@ -51,13 +51,55 @@ enableAutoPageviews(plausible);
## Automatically track outbound clicks
-To track outbound clicks automatically, call `enableAutoOutboundTracking`:
+To track outbound clicks automatically (same-origin links are skipped automatically), call `enableAutoOutboundTracking`:
```ts
import { Plausible, enableAutoOutboundTracking } from 'plausible-client';
const plausible = new Plausible({
apiHost: 'https://plausible.io',
domain: 'example.org',
});
-// Function returns cleanup callback and starts track outbound clicks
+// Function returns cleanup callback and starts tracking outbound clicks
enableAutoOutboundTracking(plausible);
+
+// Optionally capture link text or apply a custom filter
+enableAutoOutboundTracking(plausible, {
+ captureText: true,
+ filter: (url, text) => !url.includes('internal'),
+});+## Track any link clicks
+// Function returns cleanup callback
+## Score user sessions
+// Sends a "Session scored" event on the next animation frame
+You can also use Filter events@@ -155,7 +197,7 @@ const plausible = new Plausible({
|
This was referenced Mar 8, 2026
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.
Closes #52
Summary by CodeRabbit
New Features
Refactor
Tests
Chores