[PULSE-51] Visitor ID storage: optional localStorage for cross-tab unique visitors #21
Reference in New Issue
Block a user
No description provided.
Delete Branch "staging"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Work Item
PULSE-51
Summary
Changes
Test Plan
[ ] Default embed (no config): visitor ID in sessionStorage only; new tab = new session ID.
[ ] With cross-tab mode enabled: same origin, multiple tabs share one visitor ID (localStorage); counted as one visitor when backend aggregates by session_id.
[ ] With TTL set: after TTL, script regenerates ID and stores new timestamp.
[ ] Docs and comments accurately describe default, optional mode, and storage.
Greptile Overview
Greptile Summary
This PR implements configurable visitor ID storage with localStorage (cross-tab) as the default and an optional sessionStorage (per-tab) mode via
data-storage="session"attribute.Critical Issues:
|| 'local'). Comments in the code also conflict - the header says "Default: cross-tab visitor ID (localStorage)" while function comments say "Default: ephemeral (sessionStorage, per-tab)".script.jsandScriptSetupBlock.tsxwere modified. No documentation files were updated.Implementation Details:
data-storageattribute (defaults to'local'= localStorage)data-storage-ttlattribute for localStorage TTL in hours (default 24)plausible_session_key migrationAction Items:
Confidence Score: 2/5
public/script.js- the default storage mode and comments need alignment with intended behavior.Important Files Changed
data-storage="session"option. Documentation needs clarification on whether this is opting in or out of cross-tab tracking given the default behavior confusion.Sequence Diagram
2 files reviewed, 4 comments
Edit Code Review Agent Settings | Greptile
@@ -91,1 +91,4 @@</code><p className="mt-2 text-xs text-neutral-500 dark:text-neutral-400">Default: cross-tab (localStorage). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage="session"</code> to opt out (per-tab, ephemeral). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage-ttl="48"</code> to set expiry in hours (default: 24).</p>Documentation says
data-storage="session"is "Optional" but according to the implementation, this should be the default. If localStorage is now the default, this text should clarify thatdata-storage="session"opts OUT of cross-tab tracking.Prompt To Fix With AI
@@ -1,6 +1,8 @@/**Default behavior changed from sessionStorage to localStorage, contradicting PR description which states "keep sessionStorage as default". Comments say "Default: cross-tab visitor ID (localStorage)" but PR says "Default remains sessionStorage".
Prompt To Fix With AI
@@ -102,25 +109,100 @@}Comment states "Default: ephemeral (sessionStorage, per-tab)" but the actual default in line 25 is
'local'(localStorage). This is a contradiction - either the comment or the code is incorrect.Prompt To Fix With AI
@@ -122,0 +197,4 @@} catch (e) {cachedSessionId = generateId();}return cachedSessionId;Race condition fix has logic gap: if the ID in
rawAgainis expired, code falls through tolocalStorage.setItemon line 161 usingcachedSessionIdfrom line 146, but doesn't check if yet another tab wrote a fresh ID between lines 148-161. Consider re-reading one more time right before the write.Prompt To Fix With AI
@@ -1,6 +1,8 @@/**Issue: Default was documented as sessionStorage while the PR said “keep sessionStorage as default.”
Fix: Default is cross-tab (localStorage). File header now says: “Default: cross-tab visitor ID (localStorage), optional data-storage-ttl in hours. Optional: data-storage="session" for per-tab (ephemeral) counting.”
Why: Product decision is to use cross-tab as the default; docs and code are aligned with that.
@@ -102,25 +109,100 @@}Issue: Comment said “Default: ephemeral (sessionStorage, per-tab)” but the code default was 'local'.
Fix: Comment updated to: “Default: persistent (localStorage, cross-tab), optional TTL in hours. With data-storage="session": ephemeral (sessionStorage, per-tab).” Default in code remains 'local'.
Why: Comment and code now both describe the cross-tab default.
@@ -122,0 +197,4 @@} catch (e) {cachedSessionId = generateId();}return cachedSessionId;Issue: Race: we could overwrite a newer ID from another tab between the re-read and the write.
Fix: Added a final re-read of localStorage.getItem(key) immediately before localStorage.setItem. If that read returns a valid, non-expired ID, we use it and return without writing.
Why: Avoids overwriting a fresher ID written by another tab.
@@ -91,1 +91,4 @@</code><p className="mt-2 text-xs text-neutral-500 dark:text-neutral-400">Default: cross-tab (localStorage). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage="session"</code> to opt out (per-tab, ephemeral). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage-ttl="48"</code> to set expiry in hours (default: 24).</p>Issue: Copy said “Optional: data-storage="session"” without stating that the default is cross-tab and that session opts out.
Fix: Copy updated to: “Default: cross-tab (localStorage). Optional: data-storage="session" to opt out (per-tab, ephemeral).”
Why: Makes it explicit that the default is cross-tab and that data-storage="session" opts out of it.
Greptile Overview
Greptile Summary
This PR implements configurable visitor ID storage, changing the default from ephemeral sessionStorage to persistent localStorage for cross-tab visitor tracking. The implementation includes:
Key Changes:
localStoragewith a 24-hour TTL for cross-tab visitor identificationdata-storage="session"to preserve privacy-first, ephemeral behaviorIssues Found:
parseFloatcalled with radix parameter (line 27) -parseFloatonly accepts one argument, radix is forparseIntMissing Documentation:
The PR description mentions updating DATA_AUDIT.md but this file was not modified in the PR. If this file exists, it should be updated to reflect the new storage behavior and privacy implications.
Confidence Score: 4/5
Important Files Changed
Sequence Diagram
3 files reviewed, 3 comments
Edit Code Review Agent Settings | Greptile
@@ -19,6 +21,11 @@parseFloatwith two arguments (second argument is radix) is incorrect -parseFloatonly accepts one argument. The radix parameter is forparseInt, notparseFloat.Prompt To Fix With AI
@@ -137,3 +218,4 @@cachedSessionId = generateId();try {sessionStorage.setItem(key, cachedSessionId);} catch (e) {Triple-read race condition mitigation may still have a timing gap. After the third read at line 162, another tab could write between line 174 (the check) and line 175 (the write). Consider using a single atomic write with a timestamp-based winner approach, or accept that perfect synchronization isn't achievable without locks.
Prompt To Fix With AI
Generated ID at line 146 may be overwritten by re-reads but is still used if all three re-reads fail validation. If all re-reads return expired IDs, the original
cachedSessionIdfrom line 146 gets written with a fresh timestamp. This means an expired ID from another tab could cause both tabs to write different IDs. Consider regenerating the ID after the final re-read fails rather than reusing the one from line 146.Prompt To Fix With AI
Issue: When all re-reads fail, we were writing the ID generated at line 146, which could cause two tabs to write different IDs.
Fix: Call generateId() immediately before localStorage.setItem so we always write a new ID when we decide to write.
Why: Ensures we write a fresh ID for this tab and avoids reusing a stale one when all re-reads were expired/invalid.
@@ -137,3 +218,4 @@cachedSessionId = generateId();try {sessionStorage.setItem(key, cachedSessionId);} catch (e) {Issue: There is still a small window between the last re-read and the write where another tab could write.
Fix: Added a comment that this is best-effort and that perfect cross-tab sync isn’t possible without locks.
Why: We keep the re-reads to reduce the window but don’t add further reads; the comment documents the limitation.
@@ -19,6 +21,11 @@Issue: parseFloat was called with two arguments; only parseInt accepts a radix.
Fix: Removed the second argument so the call is parseFloat(script.getAttribute('data-storage-ttl') || '24').
Why: Correct use of the API.
Greptile Overview
Greptile Summary
This PR successfully implements configurable visitor ID storage with localStorage as the default for cross-tab tracking, addressing PULSE-51. The implementation includes:
data-storage="session"to use per-tab, ephemeral sessionStorage insteaddata-storage-ttlattribute (default: 24 hours) allows automatic ID regeneration after expiryKey changes:
Minor documentation gap: The
data-storage-ttlattribute is implemented but not mentioned in the user-facing UI component, which could lead to users not knowing they can customize the expiry time.Confidence Score: 4/5
Important Files Changed
Sequence Diagram
3 files reviewed, 1 comment
Edit Code Review Agent Settings | Greptile
@@ -91,1 +91,4 @@</code><p className="mt-2 text-xs text-neutral-500 dark:text-neutral-400">Default: cross-tab (localStorage). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage="session"</code> to opt out (per-tab, ephemeral). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage-ttl="48"</code> to set expiry in hours (default: 24).</p>Missing documentation for
data-storage-ttlattribute. The script supports an optional TTL parameter (default 24 hours) but this isn't mentioned here. Consider adding: "Optional:data-storage-ttl=\"48\"to set expiry in hours (default: 24)."Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
Greptile Overview
Greptile Summary
Implements cross-tab visitor tracking by switching from sessionStorage (ephemeral, per-tab) to localStorage (persistent, cross-tab) as the default, with configurable TTL for ID expiration. The implementation includes a triple-read pattern to mitigate cross-tab race conditions when multiple tabs initialize simultaneously.
Key changes:
data-storageattribute to control storage mode ('local'default or'session'for opt-out)data-storage-ttlattribute to set visitor ID expiry in hours (default: 24){id, created}timestamp for TTL enforcementOne logic issue found:
cachedSessionId), causing duplicate visitor counts until page reloadConfidence Score: 4/5
Important Files Changed
Sequence Diagram
3 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -102,25 +109,100 @@}regenerating ID after all re-reads means two tabs opening simultaneously will create different IDs even when they shouldn't - the second
generateId()call creates a new ID rather than reusing the one from line 146, causing both tabs to write their own unique IDsPrompt To Fix With AI
@@ -137,3 +218,4 @@cachedSessionId = generateId();try {sessionStorage.setItem(key, cachedSessionId);} catch (e) {when two tabs open simultaneously without any existing ID, both will reach line 176 and generate different IDs, then race to write to localStorage - one overwrites the other and tabs end up with different IDs in memory (
cachedSessionId), causing duplicate visitor counts until a page reload syncs themPrompt To Fix With AI
@@ -137,3 +218,4 @@cachedSessionId = generateId();try {sessionStorage.setItem(key, cachedSessionId);} catch (e) {Issue: Two tabs opening at once could reach the write with different IDs (especially after the extra regenerate) and race, leaving different IDs in memory and duplicate counts.
Fix: Use the ID from line 146 for the write (removed the pre-write generateId()). Kept re-reads and the best-effort comment.
Why: We only use one generated ID per tab on this path; the write race is unchanged, but we no longer add another distinct ID.
@@ -102,25 +109,100 @@}Issue: Regenerating the ID before the write meant two tabs could each get a new ID and both write, so we could end up with two different IDs and duplicate counts.
Fix: Removed the second generateId(); we now use the ID from line 146 when writing.
Why: Avoids introducing a second ID in the write path so we don’t make the simultaneous-open case worse.
@@ -91,1 +91,4 @@</code><p className="mt-2 text-xs text-neutral-500 dark:text-neutral-400">Default: cross-tab (localStorage). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage="session"</code> to opt out (per-tab, ephemeral). Optional: <code className="rounded px-1 bg-neutral-200 dark:bg-neutral-700">data-storage-ttl="48"</code> to set expiry in hours (default: 24).</p>Fix: Already added on line 93: “Optional: data-storage-ttl="48" to set expiry in hours (default: 24).”
Why: So users know they can change the cross-tab visitor ID expiry.
If the comment was from a branch that doesn’t have this yet, merge or cherry-pick the commit that added it, or re-apply that sentence in the same paragraph.
Greptile Overview
Greptile Summary
This PR updates the tracking script to support configurable visitor/session ID storage: defaulting to a cross-tab ID in
localStorage(with optional TTL viadata-storage-ttl), while allowing an opt-out to per-tab ephemeral IDs viadata-storage="session". It also updates the dashboard embed instructions and changelog entry to reflect the new unique visitor behavior.The main functional risk is in
public/script.jswhere the localStorage code path currently generates an ID twice on cold start, which can leave a tab using a different in-memorysession_idthan the one written to localStorage (and thus produce duplicate visitor counts until reload). Additionally, the changelog headings are now bracketed reference links but their reference definitions were removed, so the version headings render as dead links.Confidence Score: 2/5
Important Files Changed
Sequence Diagram
3 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -20,2 +24,2 @@[Unreleased]: https://github.com/ciphera-net/pulse/compare/v0.1.0...HEAD[0.1.0]: https://github.com/ciphera-net/pulse/releases/tag/v0.1.0[Unreleased]: https://github.com/ciphera-net/pulse/compare/v0.2.0-alpha...HEAD[0.2.0-alpha]: https://github.com/ciphera-net/pulse/compare/v0.1.0-alpha...v0.2.0-alphaMissing changelog links
This update removes the reference links at the bottom (
[Unreleased],[0.1.0]) but doesn’t add replacements for the new headings ([0.2.0-alpha],[0.1.0-alpha]). As written,## [0.2.0-alpha]and## [0.1.0-alpha]will render as dead reference links.Either keep the headings unlinked (
## 0.2.0-alpha) or re-add the reference definitions for the new versions.Prompt To Fix With AI
@@ -122,0 +179,4 @@var parsedBefore = JSON.parse(rawBeforeWrite);if (parsedBefore && typeof parsedBefore.id === 'string') {var hasValidCreatedBefore = typeof parsedBefore.created === 'number';var expBefore = ttlMs > 0 && (!hasValidCreatedBefore || (Date.now() - parsedBefore.created > ttlMs));Two IDs per tab
In the
localStoragepath, this generates an ID twice: once atcachedSessionId = generateId()(line ~146) and again atcachedSessionId = generateId()right beforelocalStorage.setItem(...)(line ~176). In the “no existing ID” case, each tab will keep the first generated ID in memory and may write a different ID tolocalStorage, so the tab’s in-memorysession_idcan differ from the stored cross-tab ID (leading to duplicate visitor counts until reload).Consider removing the second
generateId()and writing the initially generatedcachedSessionIdinstead.Prompt To Fix With AI
@@ -20,2 +24,2 @@[Unreleased]: https://github.com/ciphera-net/pulse/compare/v0.1.0...HEAD[0.1.0]: https://github.com/ciphera-net/pulse/releases/tag/v0.1.0[Unreleased]: https://github.com/ciphera-net/pulse/compare/v0.2.0-alpha...HEAD[0.2.0-alpha]: https://github.com/ciphera-net/pulse/compare/v0.1.0-alpha...v0.2.0-alphaIssue: The version headings [0.2.0-alpha] and [0.1.0-alpha] had no reference definitions at the bottom, so they rendered as dead links.
Fix: Re-added the reference link block: [Unreleased], [0.2.0-alpha], and [0.1.0-alpha] with the correct GitHub compare/release URLs.
Why: Keeps the changelog consistent with Keep a Changelog and makes the version headings clickable to the right diffs and release.
@@ -122,0 +179,4 @@var parsedBefore = JSON.parse(rawBeforeWrite);if (parsedBefore && typeof parsedBefore.id === 'string') {var hasValidCreatedBefore = typeof parsedBefore.created === 'number';var expBefore = ttlMs > 0 && (!hasValidCreatedBefore || (Date.now() - parsedBefore.created > ttlMs));Issue: In the localStorage path the ID was generated twice (once at line 146 and again before localStorage.setItem at ~176), so each tab could keep one ID in memory and write another to storage, causing duplicate visitor counts until reload.
Fix: Removed the second generateId() before localStorage.setItem. The write now uses only the ID from line 146 (cachedSessionId).
Why: Ensures one ID per tab on this path so in-memory and stored IDs stay in sync and we avoid duplicate counts from two different IDs per tab.
Greptile Overview
Greptile Summary
This PR changes the visitor/session ID strategy in the tracking script to support cross-tab unique visitor counting by default via
localStorage, with an opt-outdata-storage="session"(per-tab sessionStorage) and an optionaldata-storage-ttl(hours) to expire and regenerate the persistent ID. It also updates the dashboard’s embed helper text and the changelog to reflect the new behavior.The core logic lives in
public/script.jsinsidegetSessionId(): it reads/writes a single key (ciphera_session_id) from either localStorage or sessionStorage and then includes that value assession_idon/api/v1/eventsrequests; metrics are still sent separately using the returnedevent_id.Confidence Score: 3/5
getSessionId()currently (1) can treat IDs without acreatedtimestamp as never expiring even when TTL is configured, and (2) regenerates on non-JSON stored values rather than migrating them, which can reset visitor IDs unexpectedly. Fixing these makes behavior match the stated TTL and reduces upgrade surprises.Important Files Changed
createdmissing and missing migration for legacy plain-string stored values.Sequence Diagram
3 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -122,0 +155,4 @@// * Race fix: re-read before writing; if another tab wrote in the meantime, use that ID insteadvar rawAgain = localStorage.getItem(key);if (rawAgain) {try {Legacy value causes churn
In the
localStoragemode,localStorage.getItem(key)is alwaysJSON.parse’d; if parsing fails (including the “old string format” case mentioned in the comment), the code regenerates a new ID instead of reusing the existing string value. If any existing installs already have a plain-string ID underciphera_session_idinlocalStorage, this will force an unexpected visitor-id reset on upgrade. A small migration path (e.g., ifrawis a non-empty string and not JSON, treat it as the ID and wrap it into the JSON format withcreated) would avoid that churn.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
Additional Comments (1)
public/script.jsTTL can become ineffective
When
data-storage-ttlis set, the expiry check treats a missing/invalidcreatedtimestamp as “not expired” (ttlMs > 0 && typeof created === 'number' && ...). That means if the stored value ever lackscreated(e.g., older payloads, manual edits, or corrupted entries), the visitor ID will effectively never expire even though TTL is configured. Consider treating missing/invalidcreatedas expired (or normalizing by rewriting{id, created: Date.now()}once read).Prompt To Fix With AI
@@ -122,0 +155,4 @@// * Race fix: re-read before writing; if another tab wrote in the meantime, use that ID insteadvar rawAgain = localStorage.getItem(key);if (rawAgain) {try {Issue: In localStorage mode, a plain-string value in ciphera_session_id (e.g. from an older script) was treated as invalid and caused a new ID to be generated on upgrade.
Fix: If JSON.parse(raw) throws and raw is a non-empty string, we treat it as a legacy ID: set cachedSessionId = raw.trim(), write { id: cachedSessionId, created: Date.now() } to localStorage (first read only), and return. The same legacy handling is used in the two re-read paths (use the string as ID and return; no write there to avoid extra writes).
Why: Existing plain-string IDs are kept across upgrades and migrated to the new JSON shape in one step.
Greptile Overview
Greptile Summary
This PR changes visitor/session ID storage behavior in the tracking script to support cross-tab unique visitors by default via
localStorage, with an opt-out to per-tab ephemeralsessionStorage(data-storage="session") and an optional TTL (data-storage-ttlin hours) to refresh the stored ID.It also updates the embed snippet helper text (
components/sites/ScriptSetupBlock.tsx) to document the new defaults/options and updates the changelog for the 0.2.0-alpha release including restored reference links.The new
public/script.jslogic stores{ id, created }inlocalStorage, treats missing/invalid/expired entries as a regeneration case, and includes a small migration path for legacy plain-string IDs to reduce churn on upgrade.Confidence Score: 4/5
data-storage-ttlvalues:NaNcurrently results in no expiry, which can contradict operator expectations.Important Files Changed
Sequence Diagram
3 files reviewed, 1 comment
Edit Code Review Agent Settings | Greptile
TTL NaN disables expiry
ttlHoursis derived viaparseFloat(...); if the attribute is present but not a valid number (e.g.data-storage-ttl="24h"or empty string),parseFloatcan yieldNaN. In that casettlMsbecomes0(NaN > 0is false), and the stored{id, created}will never expire even though the user attempted to configure a TTL. Consider explicitly checkingNumber.isFinite(ttlHours)before computingttlMs, and falling back to the default (24) or treating invalid values as “no TTL”.Prompt To Fix With AI