Enhance: validate local storage (#4784)

* Validate localStorage with spec

This will help get at the source of errors like #4706 quicker

* Fix settings bugs

Close #4679 as shortcut tooltip setting is now visible.
No need to js/alert when a setting is changed. Probably leftover bugging

* Add docs and example of reusing specs in bb task

Co-authored-by: Andelf <[email protected]>

bb.edn

@@ -1,6 +1,9 @@
 {:paths ["scripts/src" "src/main"]
  :deps
- {medley/medley {:mvn/version "1.3.0"}}
+ {org.babashka/spec.alpha
+  {:git/url "https://github.com/babashka/spec.alpha"
+   :sha "1a841c4cc1d4f6dab7505a98ed2d532dd9d56b78"}
+  medley/medley {:mvn/version "1.3.0"}}
  :tasks
  {dev:watch
   logseq.tasks.dev/watch
@@ -16,6 +19,9 @@
    ;; Parallel execution - https://book.babashka.org/#parallel
    :task (run '-dev:electron-start {:parallel true})}
 
+  dev:validate-local-storage
+  logseq.tasks.spec/validate-local-storage
+
   lang:list
   logseq.tasks.lang/list-langs
 

docs/dev-practices.md

@@ -116,3 +116,15 @@ be sure to have [enabled custom
 formatters](https://github.com/binaryage/cljs-devtools/blob/master/docs/installation.md#enable-custom-formatters-in-chrome)
 in the desktop app and browser. Without this enabled, most of the log messages
 aren't readable.
+
+## Data validation and generation
+
+We currently use [spec](https://github.com/clojure/spec.alpha) for data
+validation (and generation someday). We may switch to
+[malli](https://github.com/metosin/malli) if we need to datafy our data models
+at some point.
+
+Specs should go under `src/main/frontend/spec/` and be compatible with clojure
+and clojurescript. See `frontend.spec.storage` for an example. By following
+these conventions, specs should also be usable by babashka. This is helpful as it
+allows for third party tools to be written with logseq's data model.

scripts/src/logseq/tasks/spec.clj

@@ -0,0 +1,19 @@
+(ns logseq.tasks.spec
+  "Clojure spec related tasks"
+  (:require [clojure.spec.alpha :as s]
+            [cheshire.core :as json]
+            [frontend.spec.storage :as storage-spec]
+            [clojure.edn :as edn]))
+
+;; To create file for validation, `JSON.stringify(localStorage)` in the js
+;; console and copy string to file
+(defn validate-local-storage
+  "Validate a localStorage json file"
+  [file]
+  (let [local-storage
+        (update-vals (json/parse-string (slurp file) keyword)
+                     ;; Not all localStorage values are edn so gracefully return.
+                     ;; For example, logseq-plugin-tabs stores data as json
+                     #(try (edn/read-string %) (catch Throwable _ %)))]
+    (s/assert ::storage-spec/local-storage local-storage)
+    (println "Success!")))

src/main/frontend/components/settings.cljs

@@ -603,7 +603,8 @@
      (when (util/electron?) (switch-spell-check-row t))
      (outdenting-row t logical-outdenting?)
      (when-not (or (util/mobile?) (mobile-util/is-native-platform?))
-       (shortcut-tooltip-row t enable-shortcut-tooltip?)
+       (shortcut-tooltip-row t enable-shortcut-tooltip?))
+     (when-not (or (util/mobile?) (mobile-util/is-native-platform?))
        (tooltip-row t enable-tooltip?))
      (timetracking-row t enable-timetracking?)
      (journal-row t enable-journals?)

src/main/frontend/handler/config.cljs

@@ -13,7 +13,6 @@
     (set-config! :ui/show-brackets? (not show-brackets?))))
 
 (defn toggle-logical-outdenting! []
-  (js/alert "toggle-logical-outdenting!")
   (let [logical-outdenting? (state/logical-outdenting?)]
     (set-config! :editor/logical-outdenting? (not logical-outdenting?))))
 

src/main/frontend/spec.cljs

@@ -4,8 +4,10 @@
             [lambdaisland.glogi :as log]
             [expound.alpha :as expound]))
 
-;; disable in production
-(when config/dev? (s/check-asserts true))
+;; Enabled for all environments. We want asserts to run in production e.g.
+;; frontend.storage one is preventing data corruption. If we introduce asserts
+;; that are not perf sensitive, we will need to reconsider.
+(s/check-asserts true)
 
 (set! s/*explain-out* expound/printer)
 

src/main/frontend/spec/storage.cljc

@@ -0,0 +1,59 @@
+(ns ^:bb-compatible frontend.spec.storage
+  "Specs for frontend.storage"
+  #?(:cljs (:require [cljs.spec.alpha :as s])
+     :default (:require [clojure.spec.alpha :as s])))
+
+(s/def ::db-schema map?)
+(s/def ::ls-right-sidebar-state map?)
+(s/def ::ls-right-sidebar-width string?)
+(s/def ::ls-left-sidebar-open? boolean?)
+(s/def :ui/theme string?)
+(s/def :ui/system-theme? boolean?)
+(s/def ::lsp-core-enabled boolean?)
+(s/def ::instrument-disabled boolean?)
+(s/def ::ls-pdf-area-is-dashed boolean?)
+(s/def ::ls-pdf-viewer-theme string?)
+(s/def :zotero/api-key-v2 map?)
+(s/def :zotero/setting-profile string?)
+(s/def ::commands-history (s/coll-of map?))
+(s/def :ui/wide-mode boolean?)
+(s/def :git/current-repo string?)
+(s/def ::preferred-language string?)
+(s/def ::developer-mode string?) ;; Funny string boolean
+(s/def :document/mode? boolean?)
+(s/def :ui/shortcut-tooltip? boolean?)
+(s/def :copy/export-block-text-indent-style string?)
+(s/def :copy/export-block-text-remove-options set?)
+;; Dynamic keys which aren't as easily validated:
+;; :ls-pdf-last-page-*
+;; :ls-js-allowed-*
+
+;; Validates items that are stored in local storage. The validation is approximate here
+;; e.g. we don't validate deeply into maps and collections.
+;; The namespacing is inconsistent for this map. Sometimes we use keys without
+;; namespaces and sometimes use orphaned namespaces. It would've been better
+;; if all keys were namespaced with a unique name like this one
+(s/def ::local-storage
+  ;; All these keys are optional since we usually only validate one key at a time
+  (s/keys
+   :opt-un [::db-schema
+            ::ls-right-sidebar-state
+            ::ls-right-sidebar-width
+            ::ls-left-sidebar-open?
+            :ui/theme
+            :ui/system-theme?
+            ::lsp-core-enabled
+            ::instrument-disabled
+            ::ls-pdf-area-is-dashed
+            ::ls-pdf-viewer-theme
+            :zotero/api-key-v2
+            :zotero/setting-profile
+            ::commands-history
+            :ui/wide-mode
+            :git/current-repo
+            ::preferred-language
+            ::developer-mode
+            :document/mode?
+            :ui/shortcut-tooltip?
+            :copy/export-block-text-indent-style
+            :copy/export-block-text-remove-options]))

src/main/frontend/storage.cljs

@@ -2,6 +2,8 @@
   (:refer-clojure :exclude [get set remove])
   (:require [cljs.reader :as reader]
             [datascript.transit :as dt]
+            [frontend.spec.storage :as storage-spec]
+            [cljs.spec.alpha :as s]
             [frontend.util :as util]))
 
 ;; TODO: refactor: separate side effects
@@ -12,6 +14,10 @@
 
 (defn set
   [key value]
+  ;; Prevent invalid data from being saved into storage
+  (s/assert ::storage-spec/local-storage
+            ;; Translate key to keyword for spec as not all keys are keywords
+            {(keyword key) value})
   (when-not util/node-test?
     (.setItem ^js js/localStorage (name key) (pr-str value))))