diff --git a/docs/agent-guide/039-worker-platform-abstraction-cleanup.md b/docs/agent-guide/039-worker-platform-abstraction-cleanup.md new file mode 100644 index 0000000000..2bc4e3762c --- /dev/null +++ b/docs/agent-guide/039-worker-platform-abstraction-cleanup.md @@ -0,0 +1,213 @@ +# Worker Platform Abstraction Cleanup Implementation Plan + +Goal: Route shared db-worker sync code through `frontend.worker.platform` wrappers so browser and node runtimes both work without runtime-specific branches in shared modules. + +Architecture: Keep runtime-specific APIs inside `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/platform/browser.cljs` and `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/platform/node.cljs`. +Call platform capabilities from shared modules via `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/platform.cljs` using `platform/current` at the call site. +Preserve existing key names and payload shapes to avoid data migration. + +Tech Stack: ClojureScript, promesa, cljs.test, clojure-lsp diagnostics, db-worker platform adapters. + +Related: Relates to `docs/agent-guide/038-electron-db-worker-switch-graph.md` and `docs/agent-guide/db-sync/db-sync-guide.md`. + +## Problem statement + +`frontend.worker.platform` currently exposes public wrappers that clojure-lsp reports as unused. + +The reported vars are `kv-get`, `kv-set!`, `read-text!`, `write-text!`, and `websocket-connect`. + +Shared worker modules still contain runtime-specific calls that bypass those wrappers. + +The bypasses are concentrated in `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync.cljs` and `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync/crypt.cljs`. + +This creates duplicated runtime assumptions and weakens node and browser parity. + +| Symptom | Current location | Impact | +|---|---|---| +| `clojure-lsp/unused-public-var` on `platform/kv-get` and `platform/kv-set!` | `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/platform.cljs` | Signals shared code is not using the adapter path for kv persistence. | +| `clojure-lsp/unused-public-var` on `platform/read-text!` and `platform/write-text!` | `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/platform.cljs` | Signals text file I/O in shared code can still hardcode a backend. | +| `clojure-lsp/unused-public-var` on `platform/websocket-connect` | `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/platform.cljs` | Signals websocket creation in shared sync code may bypass node adapter (`ws`). | +| Direct `js/WebSocket.` in shared sync module | `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync.cljs` | Couples shared sync lifecycle to browser global API. | +| Direct `opfs/ sync.cljs -> js/WebSocket. +db_core -> sync/crypt.cljs -> opfs/idb-keyval. + +Target shared path. +db_core -> sync.cljs -> platform/websocket-connect. +db_core -> sync/crypt.cljs -> platform/read-text!/write-text!/kv-get/kv-set!. + +Runtime adapter ownership. +browser adapter -> OPFS + IndexedDB + browser WebSocket. +node adapter -> fs + JSON kv file + ws package. +``` + +## Testing Plan + +I will add a unit test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/db_sync_test.cljs` that asserts `#'db-sync/connect!` creates sockets through `platform/websocket-connect` and not direct globals. + +I will write the test by stubbing `platform/current`, `platform/websocket-connect`, and `#'db-sync/attach-ws-handlers!` to verify the adapter call receives the tokenized URL. + +I will add unit tests in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/sync/crypt_test.cljs` that assert non-native password read and write paths call `platform/read-text!` and `platform/write-text!`. + +I will add a unit test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/sync/crypt_test.cljs` that asserts native write fallback uses `platform/write-text!` when main-thread persistence fails. + +I will add unit tests in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/sync/crypt_test.cljs` that assert encrypted AES key cache I/O flows through `platform/kv-get`, `platform/kv-set!`, and `platform/current`. + +I will run each new test case first and confirm failure before implementation changes using `@test-driven-development`. + +I will then run targeted suites and expect all tests to pass. + +I will run `bb dev:test -v frontend.worker.db-sync-test` and expect `0 failures, 0 errors`. + +I will run `bb dev:test -v frontend.worker.sync.crypt-test` and expect `0 failures, 0 errors` for the selected non-`:fix-me` cases. + +I will run `bb dev:lint-and-test` and expect lint plus unit test completion without new warnings in touched namespaces. + +I will verify clojure-lsp diagnostics no longer report `clojure-lsp/unused-public-var` for the five wrapper vars in `frontend.worker.platform`. + +NOTE: I will write *all* tests before I add any implementation behavior. + +## Scope and non-goals + +This plan changes shared worker modules that should remain runtime-agnostic. + +This plan does not change browser or node adapter implementations except where signature alignment is required. + +This plan does not redesign db-sync protocol or e2ee crypto flow. + +This plan does not migrate persisted data keys. + +This plan is intentionally limited to the exact five wrapper warnings first. + +This plan includes migrating encrypted AES key cache access in `sync/crypt.cljs` to `platform/kv-get` and `platform/kv-set!` because it is in scope of those warnings. + +This plan does not include unrelated broader idb migration outside these touched shared worker paths. + +## Implementation steps + +1. Add a failing websocket adapter test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/db_sync_test.cljs` for `#'db-sync/connect!`. + +2. Run `bb dev:test -v frontend.worker.db-sync-test` and confirm the new test fails because the code still uses direct `js/WebSocket.`. + +3. Add `frontend.worker.platform` require alias in `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync.cljs`. + +4. Replace the socket constructor in `connect!` with `(platform/websocket-connect (platform/current) ...)`. + +5. Run `bb dev:test -v frontend.worker.db-sync-test` and confirm the websocket adapter test passes. + +6. Add a failing non-native read and write adapter test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/sync/crypt_test.cljs`. + +7. Add a failing native fallback test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/sync/crypt_test.cljs` that forces native save failure and expects `platform/write-text!`. + +8. Add a failing kv adapter test in `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/sync/crypt_test.cljs` for AES key cache persistence path. + +9. Run `bb dev:test -v frontend.worker.sync.crypt-test` and confirm new tests fail before implementation. + +10. Add `frontend.worker.platform` require alias in `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync/crypt.cljs`. + +11. Replace direct password file I/O calls with `platform/read-text!` and `platform/write-text!` against `(platform/current)`. + +12. Replace direct encrypted AES cache idb calls with `platform/kv-get` and `platform/kv-set!` against `(platform/current)`. + +13. Keep key format exactly as `rtc-encrypted-aes-key###` to preserve existing browser data compatibility. + +14. Remove now-unused direct OPFS and idb-keyval requirements from `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync/crypt.cljs` if no longer referenced. + +15. Re-run `bb dev:test -v frontend.worker.sync.crypt-test` and confirm all new tests pass. + +16. Re-run `bb dev:test -v frontend.worker.db-sync-test` and confirm no regressions from sync namespace changes. + +17. Run `bb dev:lint-and-test` and confirm there are no new lint or test regressions. + +18. Verify editor or CI diagnostics no longer show the five `frontend.worker.platform` unused-public-var warnings. + +19. If any wrapper remains unused, decide whether to add a real call site or convert that wrapper to private in `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/platform.cljs`. + +20. Document verification evidence and remaining caveats in the PR description. + +## Edge cases and risk controls + +Native worker password flow must keep current behavior that first attempts main-thread persistence and falls back on local storage write. + +Node runtime may not expose browser globals, so all shared websocket construction must pass through the adapter. + +Key-value persistence must keep existing key naming to avoid cache misses for already stored encrypted graph AES keys. + +The adapter functions may return promises, so call sites must preserve existing `p/let` sequencing and error paths. + +If `platform/current` is unset in tests, failures should be explicit and tests should set a minimal platform map. + +## Clarified decisions before coding + +Encrypted AES key cache in `sync/crypt.cljs` will migrate from direct idb store to platform kv now. + +Removal of the five `unused-public-var` warnings is mandatory acceptance criteria. + +Tests remain colocated in `db_sync_test.cljs` and `sync/crypt_test.cljs` instead of adding a dedicated platform test namespace. + +## Verification commands + +```bash +bb dev:test -v frontend.worker.db-sync-test +``` + +Expected output contains the new websocket adapter test name and ends with zero failures. + +```bash +bb dev:test -v frontend.worker.sync.crypt-test +``` + +Expected output contains new adapter usage tests and ends with zero failures for executed tests. + +```bash +bb dev:lint-and-test +``` + +Expected output finishes lint plus test pipeline without new errors in touched files. + +## Skills to apply during implementation + +Use `@test-driven-development` for all behavior changes. + +Use `@clojure-debug` immediately when any new test fails unexpectedly. + +Use `@clojure-paren-repair` if Clojure delimiter errors occur while editing touched namespaces. + +## Testing Details + +Tests focus on externally visible behavior of shared worker modules choosing runtime behavior through adapter calls. + +The websocket test validates the constructor path and tokenized URL input instead of testing internal locals. + +The crypt tests validate fallback and persistence behavior through adapter interaction and returned outcomes. + +The kv cache tests validate read and write behavior by key and value flow rather than implementation-specific helpers. + +## Implementation Details + +- Touch `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync.cljs` to route socket creation through `platform/websocket-connect`. +- Touch `/Users/rcmerci/gh-repos/logseq/src/main/frontend/worker/sync/crypt.cljs` to route text and kv persistence through platform wrappers. +- Touch `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/db_sync_test.cljs` to add websocket adapter behavior tests. +- Touch `/Users/rcmerci/gh-repos/logseq/src/test/frontend/worker/sync/crypt_test.cljs` to add adapter-backed storage and kv behavior tests. +- Preserve existing e2ee key naming and payload shapes. +- Keep adapter interface unchanged unless tests prove a missing capability. +- Prefer removing obsolete direct dependencies once wrappers are adopted. +- Keep all new logic promise-safe with existing `promesa` flow. +- Validate clojure-lsp warning cleanup for all five wrapper vars. +- Keep PR scoped to abstraction usage cleanup and tests only. + +## Question + +Confirmed scope: limit this effort to the exact five wrapper warnings first. + +Confirmed decision: migrate encrypted AES key cache usage in `sync/crypt.cljs` to platform kv in this pass. + +Confirmed quality gate: the five `unused-public-var` warnings must be cleared. + +Confirmed test placement: keep new tests in existing `db_sync_test.cljs` and `sync/crypt_test.cljs`. + +--- diff --git a/package.json b/package.json index 033a117ce5..296d217187 100644 --- a/package.json +++ b/package.json @@ -210,7 +210,7 @@ "threads": "1.6.5", "url": "^0.11.0", "util": "^0.12.5", - "ws": "8.19.0", + "ws": "^8.19.0", "yargs-parser": "20.2.4" }, "resolutions": { diff --git a/src/main/frontend/worker/sync.cljs b/src/main/frontend/worker/sync.cljs index 15d0e8da19..9cf77a0e88 100644 --- a/src/main/frontend/worker/sync.cljs +++ b/src/main/frontend/worker/sync.cljs @@ -8,6 +8,7 @@ [frontend.common.crypt :as crypt] [frontend.worker-common.util :as worker-util] [frontend.worker.handler.page :as worker-page] + [frontend.worker.platform :as platform] [frontend.worker.shared-service :as shared-service] [frontend.worker.state :as worker-state] [frontend.worker.sync.client-op :as client-op] @@ -1783,7 +1784,8 @@ (stop-client! client)) ;; use cache token for faster websocket connection (when-let [token' (or token (auth-token))] - (let [ws (js/WebSocket. (append-token url token')) + (let [ws (platform/websocket-connect (platform/current) + (append-token url token')) updated (assoc client :ws ws)] (attach-ws-handlers! repo updated ws url) (set! (.-onopen ws) diff --git a/src/main/frontend/worker/sync/crypt.cljs b/src/main/frontend/worker/sync/crypt.cljs index 670cc8a07c..5f6b41aa23 100644 --- a/src/main/frontend/worker/sync/crypt.cljs +++ b/src/main/frontend/worker/sync/crypt.cljs @@ -3,9 +3,9 @@ (:require ["/frontend/idbkv" :as idb-keyval] [clojure.string :as string] [frontend.common.crypt :as crypt] - [frontend.common.file.opfs :as opfs] [frontend.common.thread-api :refer [def-thread-api]] [frontend.worker-common.util :as worker-util] + [frontend.worker.platform :as platform] [frontend.worker.state :as worker-state] [frontend.worker.sync.const :as sync-const] [lambdaisland.glogi :as log] @@ -48,14 +48,14 @@ nil) (p/catch (fn [e] (log/error :native-save-e2ee-password {:error e}) - (opfs/ r (js->clj :keywordize-keys true)))) (defn- latest-remote-tx latest-prev) (done)))))))))) +(deftest connect-uses-platform-websocket-adapter-test + (let [ws-ctor-prev js/WebSocket + platform-map {:runtime :test} + ws-calls (atom []) + attach-calls (atom [])] + (set! js/WebSocket (js* "(function(_url){ this.readyState = 1; })")) + (try + (with-redefs [worker-state/get-id-token (fn [] "token-123") + platform/current (fn [] platform-map) + platform/websocket-connect (fn [platform' url] + (swap! ws-calls conj {:platform platform' :url url}) + (js-obj)) + db-sync/attach-ws-handlers! (fn [repo _client ws url] + (swap! attach-calls conj {:repo repo :ws ws :url url}))] + (let [connected (#'db-sync/connect! test-repo {:repo test-repo} "wss://example.com/sync/graph-1") + ws (:ws connected)] + (is (= [{:platform platform-map + :url "wss://example.com/sync/graph-1?token=token-123"}] + @ws-calls)) + (is (= [{:repo test-repo + :ws ws + :url "wss://example.com/sync/graph-1"}] + @attach-calls)))) + (finally + (set! js/WebSocket ws-ctor-prev))))) + (deftest reaction-add-enqueues-pending-sync-tx-test (testing "adding a reaction should enqueue tx for db-sync" (let [{:keys [conn client-ops-conn parent]} (setup-parent-child)] diff --git a/src/test/frontend/worker/sync/crypt_test.cljs b/src/test/frontend/worker/sync/crypt_test.cljs index 25e8c6c56b..1e5f931301 100644 --- a/src/test/frontend/worker/sync/crypt_test.cljs +++ b/src/test/frontend/worker/sync/crypt_test.cljs @@ -1,6 +1,12 @@ (ns frontend.worker.sync.crypt-test (:require [cljs.test :refer [deftest is async]] + ["/frontend/idbkv" :as idb-keyval] + [clojure.string :as string] [frontend.common.crypt :as crypt] + [frontend.common.file.opfs :as opfs] + [frontend.worker-common.util :as worker-util] + [frontend.worker.platform :as platform] + [frontend.worker.state :as worker-state] [frontend.worker.sync.crypt :as sync-crypt] [logseq.db :as ldb] [promesa.core :as p])) @@ -10,6 +16,154 @@ (p/let [encrypted (crypt/ (p/with-redefs [sync-crypt/native-worker? (fn [] false) + crypt/ (p/with-redefs [sync-crypt/native-worker? (fn [] false) + platform/current (fn [] platform-map) + platform/read-text! (fn [platform' path] + (swap! read-calls conj {:platform platform' + :path path}) + (ldb/write-transit-str {:cipher "payload"})) + opfs/ (p/with-redefs [sync-crypt/native-worker? (fn [] true) + sync-crypt/ (p/with-redefs [sync-crypt/graph-e2ee? (fn [_repo] true) + sync-crypt/e2ee-base (fn [] "https://example.com") + worker-state/get-id-token (fn [] "token") + worker-util/parse-jwt (fn [_] {:sub "user-1"}) + worker-state/