diff --git a/docs/technical-guide/developer/subsystems/link-preview.md b/docs/technical-guide/developer/subsystems/link-preview.md index 52beba5c58..2f5e087624 100644 --- a/docs/technical-guide/developer/subsystems/link-preview.md +++ b/docs/technical-guide/developer/subsystems/link-preview.md @@ -51,12 +51,13 @@ link points to. The feature is therefore built from three cooperating pieces: File: `frontend/src/app/main/router.cljs` On every navigation, the `navigated` event reads the freshly stored -`(:route state)` and calls `match->context-params` to extract the -identifiers that give sharing context to the current route, and mirrors -them on the query string (before the fragment) using -`history.replaceState`. The href base comes from the canonical -`cf/public-uri`, so subpath deployments keep their prefix. The resulting -URLs look like: +`(:route state)` and syncs its `file-id`/`team-id`/`project-id` fragment +params into the query string (before the fragment) using +`history.replaceState`. Every other param in the URL is left untouched, +so unrelated params owned by other code survive. The write is skipped +when the resulting href already matches the address bar. The backend +applies its own file > project > team priority, so no filtering happens +on the frontend. The resulting URLs look like: ```text https://design.penpot.app/?file-id=#/workspace?team-id=...&file-id=...&page-id=... @@ -64,13 +65,9 @@ https://design.penpot.app/?team-id=&project-id=#/dashboard/recent?.. https://design.penpot.app/?team-id=#/dashboard/recent?team-id=... ``` -`match->context-params` implements a priority: if the route has a `file-id` -only that is mirrored; otherwise `project-id` (together with its `team-id`); -otherwise `team-id`. Routes without any of those ids (e.g. auth pages) mirror -nothing; `replaceState` only writes when the computed href differs from the -current one, so it strips a stale query string without churning the URL on -every navigation. Ids are read from the fragment `:query-params` (routes -are static screens; the only dynamic parts are the query ids). +Routes without any of those ids (e.g. auth pages) clear them from the +query string; `replaceState` only writes when the computed href differs +from the current one, so no URL churn happens on navigation. This way, when the user copies the URL from the address bar and shares it, the context ids travel in a part of the URL that *does* reach the server. @@ -288,10 +285,9 @@ indexing these preview pages, and responses are marked non-cacheable. * `backend/test/backend_tests/http_assets_test.clj` (`objects-handler-file-thumbnail-bucket-link-preview-flag`) — the `file-thumbnail` bucket is public only while the flag is enabled. - * `frontend/test/frontend_tests/router_test.cljs` — `match->context-params` - priority (file > project > team), project link without team, repeated-key - handling, and the `mirrored-href`/`navigated` URL surgery (mirror, skip, - stale-strip, clear, subpath base). + * `frontend/test/frontend_tests/router_test.cljs` — the `navigated` URL + surgery (mirror, skip, stale-strip, clear, unrelated-param + preservation, every-present-id, repeated-key, subpath base). ## Relevant files diff --git a/frontend/src/app/main/router.cljs b/frontend/src/app/main/router.cljs index 890163fb75..174635135f 100644 --- a/frontend/src/app/main/router.cljs +++ b/frontend/src/app/main/router.cljs @@ -75,34 +75,6 @@ (let [v (get params k)] (if (sequential? v) (peek v) v))) -(defn match->context-params - "Extract the params that give sharing context to the current URL. - - They are mirrored on the query string (before the fragment) because - the fragment is never sent to the server; this way shared links - carry enough context for rendering link preview metadata. - Only fragment query params are considered: non-legacy routes are - static screens and carry their ids exclusively there." - [match] - (let [query-params (get match :query-params) - file-id (get-query-param query-params :file-id) - team-id (get-query-param query-params :team-id) - project-id (get-query-param query-params :project-id)] - (cond - (some? file-id) {:file-id file-id} - (some? project-id) {:team-id team-id :project-id project-id} - (some? team-id) {:team-id team-id}))) - -(defn mirrored-href - "Build the path-relative href carrying the mirrored context query - before the fragment. Pure helper around the `navigated` effect so - the URL surgery stays testable without DOM." - [context-params hash base-path] - (let [query (some-> context-params u/map->query-string)] - (dm/str base-path - (if (some? query) (dm/str "?" query) "") - hash))) - (defn navigated [match send-event-info?] (ptk/reify ::navigated @@ -127,21 +99,23 @@ ptk/EffectEvent (effect [_ state _] - ;; The route is read from the state the `update` above just stored, - ;; not from the closed-over `match`: the effect always runs after - ;; the update. The base comes from the canonical `cf/public-uri` - ;; instead of the address bar. - (let [context (match->context-params (:route state)) - href (mirrored-href context - (.-hash globals/location) - (:path cf/public-uri)) - current (dm/str (.-pathname globals/location) - (.-search globals/location) - (.-hash globals/location))] - ;; The pre-fragment query string is owned by this mirroring: skip - ;; the write when nothing changed to avoid URL churn and dropping - ;; unrelated params set by other code. Both sides are path-relative. - (when (not= href current) + ;; The route is read from the state the `update` above just stored: + ;; the effect always runs after the update. The sharing-context ids + ;; are synced into the pre-fragment query (the fragment never reaches + ;; the server, so shared links need them there); every other param is + ;; left untouched. The backend applies its own file > project > team + ;; priority, so no filtering is needed here. + (let [params (:query-params (:route state)) + uri (u/uri (.-href globals/location)) + search (reduce (fn [m k] + (let [v (get-query-param params k)] + (if (some? v) + (assoc m k v) + (dissoc m k)))) + (u/query-string->map (:query uri)) + [:file-id :team-id :project-id]) + href (str (assoc uri :query (u/map->query-string search)))] + (when (not= href (.-href globals/location)) (.replaceState js/history nil "" href)))))) (defn navigate diff --git a/frontend/test/frontend_tests/router_test.cljs b/frontend/test/frontend_tests/router_test.cljs index f490d67bcd..a3ba3763e7 100644 --- a/frontend/test/frontend_tests/router_test.cljs +++ b/frontend/test/frontend_tests/router_test.cljs @@ -6,153 +6,116 @@ (ns frontend-tests.router-test (:require - [app.common.uri :as u] - [app.config :as cf] [app.main.router :as rt] [app.util.globals :as globals] [cljs.test :as t :include-macros true] [potok.v2.core :as ptk])) -(t/deftest match-context-params-file-link - ;; Workspace and viewer links only mirror the file-id. - (let [match {:query-params {:team-id "team-1" - :file-id "file-1" - :page-id "page-1"}}] - (t/is (= {:file-id "file-1"} - (rt/match->context-params match))))) - -(t/deftest match-context-params-project-link - (let [match {:query-params {:team-id "team-1" - :project-id "project-1"}}] - (t/is (= {:team-id "team-1" - :project-id "project-1"} - (rt/match->context-params match))))) - -(t/deftest match-context-params-team-link - (let [match {:query-params {:team-id "team-1"}}] - (t/is (= {:team-id "team-1"} - (rt/match->context-params match))))) - -(t/deftest match-context-params-no-context - (let [match {:query-params {:token "some-token"}}] - (t/is (nil? (rt/match->context-params match))))) - -(t/deftest match-context-params-project-link-without-team - ;; Without a team-id the raw map keeps a nil team-id; the nil is dropped - ;; later by the query-string serialization, not here. - (let [match {:query-params {:project-id "project-1"}}] - (t/is (= {:team-id nil - :project-id "project-1"} - (rt/match->context-params match))))) - -(t/deftest match-context-params-file-beats-project-and-team - ;; With file, project and team ids present, only the file-id is mirrored. - (let [match {:query-params {:team-id "team-1" - :project-id "project-1" - :file-id "file-1"}}] - (t/is (= {:file-id "file-1"} - (rt/match->context-params match))))) - -(t/deftest match-context-params-repeated-key - ;; A repeated query key arrives as a vector; the last value wins. - (let [match {:query-params {:file-id ["file-old" "file-1"]}}] - (t/is (= {:file-id "file-1"} - (rt/match->context-params match))))) - -(defn- with-stubbed-browser - "Run `thunk` with the `globals/location` mock props and a recording - `js/history.replaceState`. Restores both originals afterwards: the - location mock is shared across tests and `js/history` may not exist - in the test environment at all." - [{:keys [pathname search hash href]} replace-calls thunk] - (let [loc globals/location - old-pathname (.-pathname loc) - old-search (.-search loc) - old-hash (.-hash loc) - old-href (.-href loc) - old-history (.-history js/globalThis)] - (set! (.-pathname loc) pathname) - (set! (.-search loc) search) - (set! (.-hash loc) hash) +(defn- with-stubbed-href + "Run `thunk` with the `globals/location` href replaced and a recording + `js/history.replaceState`. Only the href is stubbed: the effect parses + everything it needs out of it." + [href replace-calls thunk] + (let [loc globals/location + old-href (.-href loc) + old-history (.-history js/globalThis)] (set! (.-href loc) href) (set! (.-history js/globalThis) #js {:replaceState (fn [_ _ url] (swap! replace-calls conj url))}) (try (thunk) (finally - (set! (.-pathname loc) old-pathname) - (set! (.-search loc) old-search) - (set! (.-hash loc) old-hash) (set! (.-href loc) old-href) (set! (.-history js/globalThis) old-history))))) -(t/deftest mirrored-href-subpath-base - ;; The base comes from cf/public-uri, so subpath deployments keep - ;; their prefix. - (t/is (= "/penpot/?file-id=file-1#/workspace?file-id=file-1" - (rt/mirrored-href {:file-id "file-1"} "#/workspace?file-id=file-1" "/penpot/")))) - -(t/deftest mirrored-href-no-context - ;; Routes without context clear the pre-fragment query. - (t/is (= "/#/auth/login" - (rt/mirrored-href nil "#/auth/login" "/")))) - (defn- emit-navigated - "Run the `navigated` effect with `match` stored as the state route, - pinning `cf/public-uri` so the test does not depend on the test-env - globals. The closed-over match is deliberately empty to prove the - effect reads the route from the state, not from the closure." - [match public-uri] - (with-redefs [cf/public-uri (u/uri public-uri)] - (ptk/effect (rt/navigated {} false) {:route match} nil))) + "Run the `navigated` effect with `match` stored as the state route. + The closed-over match is deliberately empty to prove the effect reads + the route from the state, not from the closure." + [match] + (ptk/effect (rt/navigated {} false) {:route match} nil)) (t/deftest navigated-mirrors-context-on-change ;; New context in the state route triggers exactly one mirrored write. (let [calls (atom [])] - (with-stubbed-browser - {:pathname "/" :search "" :hash "#/workspace?file-id=file-1" :href "http://localhost/"} + (with-stubbed-href + "http://localhost/#/workspace?file-id=file-1" calls (fn [] - (emit-navigated {:query-params {:file-id "file-1"}} "http://localhost/") - (t/is (= ["/?file-id=file-1#/workspace?file-id=file-1"] @calls)))))) + (emit-navigated {:query-params {:file-id "file-1"}}) + (t/is (= ["http://localhost/?file-id=file-1#/workspace?file-id=file-1"] @calls)))))) (t/deftest navigated-skips-write-when-mirrored ;; When the URL already carries the mirrored context, nothing is written. (let [calls (atom [])] - (with-stubbed-browser - {:pathname "/" :search "?file-id=file-1" :hash "#/workspace?file-id=file-1" :href "http://localhost/?file-id=file-1#/workspace?file-id=file-1"} + (with-stubbed-href + "http://localhost/?file-id=file-1#/workspace?file-id=file-1" calls (fn [] - (emit-navigated {:query-params {:file-id "file-1"}} "http://localhost/") + (emit-navigated {:query-params {:file-id "file-1"}}) (t/is (= [] @calls)))))) (t/deftest navigated-strips-stale-context ;; A stale pre-fragment query is replaced with the current context. (let [calls (atom [])] - (with-stubbed-browser - {:pathname "/" :search "?file-id=old" :hash "#/dashboard/recent?team-id=team-1" :href "http://localhost/?file-id=old#/dashboard/recent?team-id=team-1"} + (with-stubbed-href + "http://localhost/?file-id=old#/dashboard/recent?team-id=team-1" calls (fn [] - (emit-navigated {:query-params {:team-id "team-1"}} "http://localhost/") - (t/is (= ["/?team-id=team-1#/dashboard/recent?team-id=team-1"] @calls)))))) + (emit-navigated {:query-params {:team-id "team-1"}}) + (t/is (= ["http://localhost/?team-id=team-1#/dashboard/recent?team-id=team-1"] @calls)))))) (t/deftest navigated-clears-query-without-context ;; Routes without context clear a stale pre-fragment query. (let [calls (atom [])] - (with-stubbed-browser - {:pathname "/" :search "?file-id=old" :hash "#/auth/login" :href "http://localhost/?file-id=old#/auth/login"} + (with-stubbed-href + "http://localhost/?file-id=old#/auth/login" calls (fn [] - (emit-navigated {:query-params {:token "some-token"}} "http://localhost/") - (t/is (= ["/#/auth/login"] @calls)))))) + (emit-navigated {:query-params {:token "some-token"}}) + (t/is (= ["http://localhost/#/auth/login"] @calls)))))) + +(t/deftest navigated-preserves-unrelated-params + ;; Params owned by other code are kept as they are. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/?debug=1&file-id=old#/workspace?file-id=file-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id "file-1"}}) + (t/is (= ["http://localhost/?debug=1&file-id=file-1#/workspace?file-id=file-1"] @calls)))))) + +(t/deftest navigated-mirrors-every-present-context-id + ;; Every present context id is mirrored; the backend applies its own + ;; file > project > team priority, so no filtering happens here. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/#/workspace?file-id=file-1&team-id=team-1&project-id=project-1&page-id=page-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id "file-1" + :team-id "team-1" + :project-id "project-1" + :page-id "page-1"}}) + (t/is (= ["http://localhost/?file-id=file-1&team-id=team-1&project-id=project-1#/workspace?file-id=file-1&team-id=team-1&project-id=project-1&page-id=page-1"] + @calls)))))) + +(t/deftest navigated-repeated-key-last-wins + ;; A repeated query key arrives as a vector; the last value wins. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/#/workspace?file-id=file-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id ["file-old" "file-1"]}}) + (t/is (= ["http://localhost/?file-id=file-1#/workspace?file-id=file-1"] @calls)))))) (t/deftest navigated-keeps-subpath-base - ;; Under a subpath deployment the mirrored href keeps the prefix - ;; from cf/public-uri. + ;; Under a subpath deployment the prefix survives untouched. (let [calls (atom [])] - (with-stubbed-browser - {:pathname "/penpot/" :search "" :hash "#/workspace?file-id=file-1" :href "http://localhost/penpot/"} + (with-stubbed-href + "http://localhost/penpot/#/workspace?file-id=file-1" calls (fn [] - (emit-navigated {:query-params {:file-id "file-1"}} "http://localhost/penpot/") - (t/is (= ["/penpot/?file-id=file-1#/workspace?file-id=file-1"] @calls)))))) + (emit-navigated {:query-params {:file-id "file-1"}}) + (t/is (= ["http://localhost/penpot/?file-id=file-1#/workspace?file-id=file-1"] @calls))))))