Fix storage quota bug #489

Pete-Y-CS · 2024-03-28T14:25:49Z

No description provided.

Pete-Y-CS · 2024-03-28T14:27:19Z

Ahh just remembered you wanted us to display the amount of memory used, let me add that now

dabreegster · 2024-03-28T14:52:53Z

And is a playwright test possible?

Pete-Y-CS · 2024-03-28T14:59:06Z

I tried to add a breaking length string in playwright and it timed out. Maybe there's another way but I think it would be easier with unit rather than functional tests.

window.alert doesn't seem to reliably appear, so try a Modal instead

dabreegster · 2024-04-01T19:49:23Z

Not sure why a 1MB file is necessary for a test when we can do "x".repeat(123). Also don't think we need to force a browser to deal with 5MB in a form. We can just set some unused local storage key directly. I started a test in 7168bab. But the window.alerts appear to be quite flaky both in playwright and manually; they mostly haven't appeared for me, which is weird, because I thought window.alert is supposed to be blocking. What if we try a Modal instead and just have this handling in the sketch tool for now?

robinlovelace-ate · 2024-04-02T08:08:06Z

Looks like a tricky one, happy to test locally if you guys want a second pair of eyes on it.

dabreegster · 2024-04-03T10:48:01Z

src/lib/common/storage.ts

+  } catch (error: any) {
+    // Unfortunately it looks like we can't use typeof or instanceof to determine this one:
+    // all we get is that it's an object so if someone changess the wording we may have to add new checks
+    const isStorageQuotaError =


I'd vote for getting rid of all of this logic entirely to simplify things. Catch any exception and assume it's a quota problem. What other exceptions might be thrown? In Firefox, the message is different:

dabreegster · 2024-04-03T10:51:04Z

src/lib/common/storage.ts

+export function clearLocalStorage() {
+  window.localStorage.clear();
+}
+export function clearLocalStorageItem(key: string) {


Missing return type here. But actually, it might be simpler to not try and return this here. Export getStoredStrings and call it from the modal. After clicking a button to clear something, over in the modal code, call the function again to refresh the list.

Then we don't need getEmptySetStorageErrorObject or maybe even the setLocalStorageItem wrapper at all. Just call the regular API, catch any error, launch the modal

dabreegster · 2024-04-03T10:52:44Z

src/lib/common/storage.ts

@@ -0,0 +1,77 @@
+const storageQuotaErrorMessage =


The screenfull of text is a bit overwhelming:

I'm going to make some wording suggestions below

dabreegster · 2024-04-03T10:54:20Z

src/lib/common/storage.ts

@@ -0,0 +1,77 @@
+const storageQuotaErrorMessage =
+  "Unable to save because the local storage quota has been exceeded: you may need to clear out your web browser's local storage for this app, or  run in private mode before you can save again. Specific error here: ";


I'd vote to move this to the modal. We don't need to console.log it, it's user-facing

dabreegster · 2024-04-03T10:57:12Z

src/lib/sidebar/StorageModal.svelte

+      {#if storedStringDescriptor.key === currentAuthority}
+        <p><WarningIcon text={"warning icon"} />This is the current sketch.</p>
+      {/if}
+      <SecondaryButton on:click={removeStorageItem(storedStringDescriptor.key)}>


If you do () => removeStorageItem(key) then we don't need the weird indirection of returning a closure. Most Svelte examples make the closure here in the event handler

an obvious trick I'm sure I've learned before that slipped my brain, thanks

dabreegster · 2024-04-03T11:01:28Z

src/lib/sidebar/StorageModal.svelte

+    Be sure that you're not losing anything important before doing this!
+  </p>
+  <WarningButton on:click={clearLocalStorageAndCloseModal}>
+    Delete All Sketch Data From Browser


Don't capitalize every word

dabreegster · 2024-04-03T11:02:08Z

src/lib/sidebar/StorageModal.svelte

+      <p>Stored object: {storedStringDescriptor.key}</p>
+      <p>
+        Storage used (rounded to nearest 0.01MB): {storedStringDescriptor.storageUsedInMB
+          .toString()


slice is weird -- toFixed(2)?

dabreegster · 2024-04-03T11:02:41Z

src/lib/sidebar/StorageModal.svelte

+      Otherwise here is a breakdown of what is currently stored locally. You can
+      delete individual items (normally storage quota is 5mb or 10mb):
+    </p>
+    {#each setStorageError.storedStrings as storedStringDescriptor}


It could be useful to show these from largest to smallest

dabreegster · 2024-04-03T11:05:39Z

src/lib/common/storage.ts

+  return results;
+}
+
+function getLengthInMB(text: string) {


Missing return type, and you don't need the intermediate variable. Every intermediate var is something that needs a name and takes mental overhead to understand when reading

dabreegster · 2024-04-03T11:08:14Z

src/lib/common/storage.ts

+}
+
+function getStoredStrings(): StoredStringDescriptor[] {
+  const itemsObject = { ...localStorage };


This does seem to work, but it doesn't seem documented at https://developer.mozilla.org/en-US/docs/Web/API/Storage. Alternatively there's length and key, like https://github.com/dabreegster/ltn/blob/47187e79fb5bcddc823a6224a1f80d39d624f9b2/web/src/title/TitleMode.svelte#L30

dabreegster · 2024-04-03T11:10:11Z

src/lib/sidebar/StorageModal.svelte

+      delete individual items (normally storage quota is 5mb or 10mb):
+    </p>
+    {#each setStorageError.storedStrings as storedStringDescriptor}
+      <p>Stored object: {storedStringDescriptor.key}</p>


This is repetitive and visually confusing with a bunch of keys. Why not just have a bunch of buttons, keep the key in there, and add (123MB) at the end or something?

dabreegster

Sorted buttons feel much nicer, thanks! Still some open comments about simplifying the wording

dabreegster · 2024-04-04T08:53:25Z

src/lib/sidebar/StorageModal.svelte

+      <SecondaryButton
+        on:click={() => removeStorageItem(storedStringDescriptor.key)}
+      >
+        Remove stored item for {storedStringDescriptor.key}({storedStringDescriptor.storageUsedInMB}MB)


.toFixed(2) on the number would look nicer and I think match the old slice previously

Yep, did this at first then it got lost in refactor of how we're displaying this! Good spot!

dabreegster · 2024-04-04T08:55:07Z

tests/quota.spec.ts

+    window.localStorage.setItem("huge", "x".repeat(4.999 * 1024 * 1024)),
+  );
+
+  await page.goto("/index.html?schema=pipeline");


Could just go directly to the scheme page with the right query, to speed up the test. We test navigating from the homepage in other test cases, and local storage shouldn't really affect that

dabreegster · 2024-04-04T08:58:15Z

src/lib/common/storage.ts

+const storageQuotaErrorMessage =
+  "Unable to save because the local storage quota has been exceeded: you may need to clear out your web browser's local storage for this app, or  run in private mode before you can save again. Specific error here: ";
+
+export interface SetStorageErrorObject {


We maybe don't really need this, the empty object method, or setLocalStorageItem. The caller in FileManagement could call localStorage.setItem directly, catch the exception, and trigger the modal when appropriate. We could export the method to summarize current local storage items, and make the modal call it again after clearing/removing an item. But up to you

As mentioned in teams, I think it's plasuible we'd want to share this in the future and I prefer smaller files so I'd prefer to leave it this way.

Peter York added 6 commits March 27, 2024 13:56

First attempt to test overfilling the storage

6f905bb

Add an alert when a user overfills local storage one way or anotehr

f541a3f

NPM run fmt

8aae523

remove now unused test file

5f2e0d2

Remove unnecessary check

02a0697

Add explantory comment

a0ea297

Pete-Y-CS requested a review from dabreegster March 28, 2024 14:25

Peter York and others added 2 commits March 28, 2024 14:59

Add breakdown of used storage as a second alert

dec3203

Start a test for quota limits

7168bab

window.alert doesn't seem to reliably appear, so try a Modal instead

Peter York added 6 commits April 2, 2024 14:28

Modal on sketch page for when storage is overloaded

483d870

Update test

dbd40ff

first pass at quota delete all test

4126914

set something before trying to make a route

f918a05

Reload page before each test

e934d00

Add test of deleting a single local storage item

adc0134

Pete-Y-CS force-pushed the fix-storage-quota-bug branch from b8bce1c to adc0134 Compare April 3, 2024 10:07

Add a before each to clear local storage

b370ac7

dabreegster reviewed Apr 3, 2024

View reviewed changes

Peter York added 2 commits April 3, 2024 12:30

Begin to address PR comments

c7eb0fe

Sort stored items

a3d2f3c

Pete-Y-CS force-pushed the fix-storage-quota-bug branch from 472239d to 17750c3 Compare April 3, 2024 12:53

Npm run fmt and change beforeEach

c0c7aa3

Pete-Y-CS force-pushed the fix-storage-quota-bug branch from 17750c3 to c0c7aa3 Compare April 3, 2024 12:57

dabreegster reviewed Apr 4, 2024

View reviewed changes

Peter York added 2 commits April 4, 2024 10:59

A couple of tweaks for PR comments and trying to get last test working

463864d

Npm run fmt and get last test running

6301ac0

dabreegster approved these changes Apr 4, 2024

View reviewed changes

Pete-Y-CS merged commit b70cd47 into main Apr 4, 2024
2 checks passed

Pete-Y-CS deleted the fix-storage-quota-bug branch April 4, 2024 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix storage quota bug #489

Fix storage quota bug #489

Pete-Y-CS commented Mar 28, 2024

Pete-Y-CS commented Mar 28, 2024

dabreegster commented Mar 28, 2024

Pete-Y-CS commented Mar 28, 2024

dabreegster commented Apr 1, 2024

robinlovelace-ate commented Apr 2, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

Pete-Y-CS Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster Apr 3, 2024

dabreegster left a comment

dabreegster Apr 4, 2024

Pete-Y-CS Apr 4, 2024

dabreegster Apr 4, 2024

dabreegster Apr 4, 2024

Pete-Y-CS Apr 4, 2024

		@@ -0,0 +1,77 @@
		const storageQuotaErrorMessage =
		"Unable to save because the local storage quota has been exceeded: you may need to clear out your web browser's local storage for this app, or run in private mode before you can save again. Specific error here: ";

Fix storage quota bug #489

Fix storage quota bug #489

Conversation

Pete-Y-CS commented Mar 28, 2024

Pete-Y-CS commented Mar 28, 2024

dabreegster commented Mar 28, 2024

Pete-Y-CS commented Mar 28, 2024

dabreegster commented Apr 1, 2024

robinlovelace-ate commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment