mirror of
https://github.com/penpot/penpot.git
synced 2026-09-10 05:58:47 +00:00
♻️ Sync context ids surgically instead of rebuilding the href
Replace match->context-params and mirrored-href with an in-place sync of the file-id, team-id and project-id keys on the parsed href: present ids are set, absent ones removed, every other param untouched. The backend keeps applying its own file, project and team priority. Rewrite router tests around a href-only stub: mirror, skip, stale-strip, clear, unrelated-param preservation, every-present-id, repeated-key and subpath cases. Update the link-preview subsystem doc. AI-assisted-by: muse-spark-1.3-contributor
This commit is contained in:
Andrey Antukh
parent
5913c1bb14
commit
4fc177ae43
@ -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=<uuid>#/workspace?team-id=...&file-id=...&page-id=...
|
||||
@ -64,13 +65,9 @@ https://design.penpot.app/?team-id=<uuid>&project-id=<uuid>#/dashboard/recent?..
|
||||
https://design.penpot.app/?team-id=<uuid>#/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
|
||||
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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))))))
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user