ciphera-net/pulse
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 15 Wiki Activity

[PULSE-51] Visitor ID storage: optional localStorage for cross-tab unique visitors #21

Merged
uz1mani merged 17 commits from staging into main 2026-02-11 15:14:13 +00:00
Conversation 28 Commits 17 Files Changed 3 +108 -18
3 changed files with 108 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 4a8f627a5c - Show all commits

32
public/script.js
View File

@@ -133,14 +133,22 @@
try {
const parsed = JSON.parse(raw);
if (parsed && typeof parsed.id === 'string') {
const expired = ttlMs > 0 && typeof parsed.created === 'number' && (Date.now() - parsed.created > ttlMs);
const hasValidCreated = typeof parsed.created === 'number';
const expired = ttlMs > 0 && (!hasValidCreated || (Date.now() - parsed.created > ttlMs));
if (!expired) {
cachedSessionId = parsed.id;
return cachedSessionId;
}
}
} catch (e) {
// * Invalid JSON or old string format: treat as expired and regenerate
// * Invalid JSON: migrate legacy plain-string ID to { id, created } format
if (typeof raw === 'string' && raw.trim().length > 0) {
cachedSessionId = raw.trim();
try {
localStorage.setItem(key, JSON.stringify({ id: cachedSessionId, created: Date.now() }));
} catch (e2) {}
return cachedSessionId;
}
}
}
cachedSessionId = generateId();
@@ -150,13 +158,19 @@
try {
greptile-apps[bot] commented 2026-02-11 14:48:36 +00:00 (Migrated from github.com)
Review
Copy Link

Legacy value causes churn
In the localStorage mode, localStorage.getItem(key) is always JSON.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 under ciphera_session_id in localStorage, this will force an unexpected visitor-id reset on upgrade. A small migration path (e.g., if raw is a non-empty string and not JSON, treat it as the ID and wrap it into the JSON format with created) 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 
This is a comment left during a code review.
Path: public/script.js
Line: 129:132

Comment:
**Legacy value causes churn**
In the `localStorage` mode, `localStorage.getItem(key)` is always `JSON.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 under `ciphera_session_id` in `localStorage`, this will force an unexpected visitor-id reset on upgrade. A small migration path (e.g., if `raw` is a non-empty string and not JSON, treat it as the ID and wrap it into the JSON format with `created`) would avoid that churn.

<sub>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!</sub>

How can I resolve this? If you propose a fix, please make it concise.
**Legacy value causes churn** In the `localStorage` mode, `localStorage.getItem(key)` is always `JSON.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 under `ciphera_session_id` in `localStorage`, this will force an unexpected visitor-id reset on upgrade. A small migration path (e.g., if `raw` is a non-empty string and not JSON, treat it as the ID and wrap it into the JSON format with `created`) would avoid that churn. <sub>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!</sub> <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: public/script.js Line: 129:132 Comment: **Legacy value causes churn** In the `localStorage` mode, `localStorage.getItem(key)` is always `JSON.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 under `ciphera_session_id` in `localStorage`, this will force an unexpected visitor-id reset on upgrade. A small migration path (e.g., if `raw` is a non-empty string and not JSON, treat it as the ID and wrap it into the JSON format with `created`) would avoid that churn. <sub>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!</sub> How can I resolve this? If you propose a fix, please make it concise. ````` </details>
uz1mani commented 2026-02-11 14:52:23 +00:00 (Migrated from github.com)
Review
Copy Link

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.

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.
var parsedAgain = JSON.parse(rawAgain);
if (parsedAgain && typeof parsedAgain.id === 'string') {
var expiredAgain = ttlMs > 0 && typeof parsedAgain.created === 'number' && (Date.now() - parsedAgain.created > ttlMs);
var hasValidCreatedAgain = typeof parsedAgain.created === 'number';
var expiredAgain = ttlMs > 0 && (!hasValidCreatedAgain || (Date.now() - parsedAgain.created > ttlMs));
if (!expiredAgain) {
cachedSessionId = parsedAgain.id;
return cachedSessionId;
}
}
} catch (e2) {}
} catch (e2) {
if (typeof rawAgain === 'string' && rawAgain.trim().length > 0) {
cachedSessionId = rawAgain.trim();
return cachedSessionId;
}
}
}
// * Final re-read immediately before write to avoid overwriting a fresher ID from another tab
var rawBeforeWrite = localStorage.getItem(key);
@@ -164,13 +178,19 @@
try {
var parsedBefore = JSON.parse(rawBeforeWrite);
if (parsedBefore && typeof parsedBefore.id === 'string') {
var expBefore = ttlMs > 0 && typeof parsedBefore.created === 'number' && (Date.now() - parsedBefore.created > ttlMs);
var hasValidCreatedBefore = typeof parsedBefore.created === 'number';
var expBefore = ttlMs > 0 && (!hasValidCreatedBefore || (Date.now() - parsedBefore.created > ttlMs));
greptile-apps[bot] commented 2026-02-11 14:38:11 +00:00 (Migrated from github.com)
Review
Copy Link

Two IDs per tab

In the localStorage path, this generates an ID twice: once at cachedSessionId = generateId() (line ~146) and again at cachedSessionId = generateId() right before localStorage.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 to localStorage, so the tab’s in-memory session_id can differ from the stored cross-tab ID (leading to duplicate visitor counts until reload).

Consider removing the second generateId() and writing the initially generated cachedSessionId instead.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: public/script.js
Line: 146:148

Comment:
**Two IDs per tab**

In the `localStorage` path, this generates an ID twice: once at `cachedSessionId = generateId()` (line ~146) and again at `cachedSessionId = generateId()` right before `localStorage.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 to `localStorage`, so the tab’s in-memory `session_id` can differ from the stored cross-tab ID (leading to duplicate visitor counts until reload).

Consider removing the second `generateId()` and writing the initially generated `cachedSessionId` instead.

How can I resolve this? If you propose a fix, please make it concise.
**Two IDs per tab** In the `localStorage` path, this generates an ID twice: once at `cachedSessionId = generateId()` (line ~146) and again at `cachedSessionId = generateId()` right before `localStorage.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 to `localStorage`, so the tab’s in-memory `session_id` can differ from the stored cross-tab ID (leading to duplicate visitor counts until reload). Consider removing the second `generateId()` and writing the initially generated `cachedSessionId` instead. <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: public/script.js Line: 146:148 Comment: **Two IDs per tab** In the `localStorage` path, this generates an ID twice: once at `cachedSessionId = generateId()` (line ~146) and again at `cachedSessionId = generateId()` right before `localStorage.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 to `localStorage`, so the tab’s in-memory `session_id` can differ from the stored cross-tab ID (leading to duplicate visitor counts until reload). Consider removing the second `generateId()` and writing the initially generated `cachedSessionId` instead. How can I resolve this? If you propose a fix, please make it concise. ````` </details>
uz1mani commented 2026-02-11 14:44:20 +00:00 (Migrated from github.com)
Review
Copy Link

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.

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.
if (!expBefore) {
cachedSessionId = parsedBefore.id;
return cachedSessionId;
}
}
} catch (e3) {}
} catch (e3) {
if (typeof rawBeforeWrite === 'string' && rawBeforeWrite.trim().length > 0) {
cachedSessionId = rawBeforeWrite.trim();
return cachedSessionId;
}
}
}
// * Best-effort only: another tab could write between here and setItem; without locks perfect sync is not achievable
localStorage.setItem(key, JSON.stringify({ id: cachedSessionId, created: Date.now() }));