Skip to content

Clearing expired local storage items on initialization #98

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 17, 2025
Merged

Clearing expired local storage items on initialization #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d639a62
clearing expired local storage items on init
djanhjo Oct 15, 2025
cdd713a
fix
djanhjo Oct 15, 2025
edd032c
feedback
djanhjo Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/gist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import EventEmitter from "./utilities/event-emitter";
import { log } from "./utilities/log";
import { clearExpiredFromLocalStore } from "./utilities/local-storage";
import { startQueueListener, checkMessageQueue, stopSSEListener } from "./managers/queue-manager";
import { setUserToken, clearUserToken, useGuestSession } from "./managers/user-manager";
import { showMessage, embedMessage, hideMessage, removePersistentMessage, fetchMessageByInstanceId, logBroadcastDismissedLocally } from "./managers/message-manager";
Expand All @@ -23,6 +24,7 @@ export default class {
this.currentRoute = null;
this.isDocumentVisible = true;
this.config.isPreviewSession = setupPreview();
clearExpiredFromLocalStore();

log(`Setup complete on ${this.config.env} environment.`);

Expand Down
64 changes: 44 additions & 20 deletions src/utilities/local-storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { log } from "./log";

const maxExpiryDays = 365;

const isPersistingSessionLocalStoreName = "gist.web.isPersistingSession";
Expand All @@ -21,32 +23,20 @@ export function setKeyToLocalStore(key, value, ttl = null) {
}

export function getKeyFromLocalStore(key) {
const itemStr = getStorage().getItem(key);
if (!itemStr) return null;

const item = JSON.parse(itemStr);
const now = new Date();
const expiryTime = new Date(item.expiry);

// Retroactive bugfix: remove old cache entries with long expiry times
const isBroadcastOrUserKey = (key.startsWith("gist.web.message.broadcasts") && !key.endsWith("shouldShow") && !key.endsWith("numberOfTimesShown")) || (key.startsWith("gist.web.message.user") && !key.endsWith("seen"));
const sixtyMinutesFromNow = new Date(now.getTime() + 61 * 60 * 1000);
if (isBroadcastOrUserKey && expiryTime.getTime() > sixtyMinutesFromNow.getTime()) {
clearKeyFromLocalStore(key);
return null;
}

if (now.getTime() > expiryTime.getTime()) {
clearKeyFromLocalStore(key);
return null;
}
return item.value;
return checkKeyForExpiry(key);
}

export function clearKeyFromLocalStore(key) {
getStorage().removeItem(key);
}

export function clearExpiredFromLocalStore() {
const storage = getStorage();
for (let i = storage.length - 1; i >= 0; i--) {
checkKeyForExpiry(storage.key(i));
}
}
Copy link

@cursor cursor bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Storage Iteration Bug Causes Item Skipping

The clearExpiredFromLocalStore function modifies storage during iteration. When checkKeyForExpiry removes an expired item, subsequent items shift indices, causing the loop to skip keys. This means some expired items may not be cleared as intended.

Fix in Cursor Fix in Web

Copy link
Collaborator

@BernardGatt BernardGatt Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch Cursor, I think this can be an issue @djanhjo

Copy link
Contributor Author

@djanhjo djanhjo Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be outdated since I reversed the for loop.

Copy link
Collaborator

@BernardGatt BernardGatt Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we, just out of safety, check the name and if it starts with gist.web we delete it? I'm just worried we might delete something else. All keys should start with that as far as I know.

Copy link
Contributor Author

@djanhjo djanhjo Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In its current form, it'll only potentially delete gist.web.message.broadcasts and gist.web.message.user items based on:

const isBroadcastOrUserKey = (key.startsWith("gist.web.message.broadcasts") && !key.endsWith("shouldShow") && !key.endsWith("numberOfTimesShown")) 
  || (key.startsWith("gist.web.message.user") && !key.endsWith("seen"));

Is this fine or should the logic be changed to review all gist.web items in local storage and not just gist.web.message.broadcasts / gist.web.message.user?

Also if it helps, I also prompted a review on other storage items in the repo:

gist.web.
├── isPersistingSession              (session/localStorage switcher)
├── userToken                        (30-day TTL)
├── usingGuestUserToken             (boolean flag)
├── guestUserToken                  (365-day TTL)
├── userLocale                      (no explicit TTL, uses default 365 days)
├── customAttributes                (30-day TTL)
├── userQueueUseSSE                 (1-minute TTL)
├── activeSSEConnection             (heartbeat-based TTL)
├── userQueueNextPullCheck          (dynamic TTL based on polling interval)
├── sessionId                       (30-minute TTL)
└── message.*                       (user-specific message data)
    ├── broadcasts.{hashedUserToken}.*
    │   ├── [messageData]           (60-minute TTL)
    │   ├── {queueId}.shouldShow    (varies: immediate or delayed)
    │   └── {queueId}.numberOfTimesShown (permanent until user changes)
    └── user.{hashedUserToken}.*
        ├── [userMessages]          (60-minute TTL)
        ├── seen                    (permanent list)
        └── message.{queueId}.loading (5-second TTL)

From my minimal knowledge, it doesn't seem like the other keys are worth cleaning up as they seem to just re-populate, but I might be missing context!

Copy link
Collaborator

@BernardGatt BernardGatt Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you're right, those are the worst offenders. Approving! Thank you for this 🙏

Copy link

@cursor cursor bot Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Storage Cleanup Fails on Malformed Entries

The clearExpiredFromLocalStore function iterates through all storage keys, assuming they are valid JSON with an expiry property. If storage contains non-JSON or malformed entries (e.g., from other scripts), JSON.parse throws an error, halting the cleanup and potentially breaking application setup.

Fix in Cursor Fix in Web

Copy link
Collaborator

@BernardGatt BernardGatt Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we should look at as well, we're sharing the local store with the entire app.


export function isSessionBeingPersisted() {
const currentValue = sessionStorage.getItem(isPersistingSessionLocalStoreName);
if (currentValue === null) {
Expand All @@ -59,4 +49,38 @@ export function isSessionBeingPersisted() {
// Helper function to select the correct storage based on the session flag
function getStorage() {
return isSessionBeingPersisted() ? localStorage : sessionStorage;
}

function checkKeyForExpiry(key) {
if (!key) return null;

try {
const itemStr = getStorage().getItem(key);
if (!itemStr) return null;

const item = JSON.parse(itemStr);
if (!item.expiry) return item.value;

const now = new Date();
const expiryTime = new Date(item.expiry);

// remove old cache entries with long expiry times
const isBroadcastOrUserKey = (key.startsWith("gist.web.message.broadcasts") && !key.endsWith("shouldShow") && !key.endsWith("numberOfTimesShown")) || (key.startsWith("gist.web.message.user") && !key.endsWith("seen"));
const sixtyMinutesFromNow = new Date(now.getTime() + 61 * 60 * 1000);
if (isBroadcastOrUserKey && expiryTime.getTime() > sixtyMinutesFromNow.getTime()) {
clearKeyFromLocalStore(key);
return null;
}

if (now.getTime() > expiryTime.getTime()) {
clearKeyFromLocalStore(key);
return null;
}

return item.value;
} catch (e) {
log(`Error checking key ${key} for expiry: ${e}`);
}

return null;
}