github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-09-10 05:58:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

♻️ Simplify router context mirroring and remove legacy routes

Browse Source 
Read the route from state in the navigated effect instead of the closed-over match, and build the href base from cf/public-uri instead of location.pathname, via the new pure mirrored-href helper.

Drop the legacy path-param fallback from match->context-params and remove the legacy hash routes (viewer-legacy, workspace-legacy, dashboard-legacy-*) with their redirect components; old URLs now resolve to not-found. render-sprite is kept.

Update the link-preview subsystem doc and extend router tests (subpath base, query clearing, state-as-source).

AI-assisted-by: muse-spark-1.3-contributor
This commit is contained in:
Andrey Antukh 2026-09-09 18:10:56 +00:00
parent 9e8e2c4a2a
commit 80fc70c211
5 changed files with 91 additions and 183 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
docs/technical-guide/developer/subsystems/link-preview.md
View File

@ -50,10 +50,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 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 resulting URLs look like:
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:
```text
https://design.penpot.app/?file-id=<uuid>#/workspace?team-id=...&file-id=...&page-id=...
@ -66,13 +69,16 @@ 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 both
from `:query-params` (current routes) and from `[:params :path]` (legacy
routes that carry them as path params).
every navigation. Ids are read from the fragment `:query-params` (routes
are static screens; the only dynamic parts are the query ids).
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.
Legacy hash routes (`/workspace/:project-id/:file-id`, `/view/:file-id`,
`/dashboard/team/:team-id/...`) were removed: those old URLs no longer
redirect and resolve to the not-found page instead.
### 2. Nginx: detecting link preview crawlers
Files: `docker/devenv/files/nginx.conf` (devenv) and
@ -282,9 +288,10 @@ 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, and legacy
path-params support.
* `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).
## Relevant files

41
frontend/src/app/main/router.cljs
View File

@ -80,21 +80,29 @@
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."
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 [path-params (dm/get-in match [:params :path])
query-params (get match :query-params)
file-id (or (get-query-param query-params :file-id)
(get-query-param path-params :file-id))
team-id (or (get-query-param query-params :team-id)
(get-query-param path-params :team-id))
project-id (or (get-query-param query-params :project-id)
(get-query-param path-params :project-id))]
(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
@ -118,12 +126,15 @@
(dissoc :exception)))
ptk/EffectEvent
(effect [_ _ _]
(let [query (some-> (match->context-params match)
(u/map->query-string))
href (dm/str (.-pathname globals/location)
(if (some? query) (dm/str "?" query) "")
(.-hash globals/location))
(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))]

131
frontend/src/app/main/ui.cljs
View File

@ -6,21 +6,16 @@
(ns app.main.ui
(:require
[app.common.data :as d]
[app.common.uuid :as uuid]
[app.config :as cf]
[app.main.data.common :as dcm]
[app.main.data.nitrate :as dnt]
[app.main.data.team :as dtm]
[app.main.errors :as errors]
[app.main.refs :as refs]
[app.main.repo :as rp]
[app.main.router :as rt]
[app.main.store :as st]
[app.main.ui.context :as ctx]
[app.main.ui.debug.icons-preview :refer [icons-preview*]]
[app.main.ui.debug.playground :refer [playground*]]
[app.main.ui.ds.product.loader :refer [loader*]]
[app.main.ui.error-boundary :refer [error-boundary*]]
[app.main.ui.exports.files]
[app.main.ui.frame-preview :as frame-preview]
@ -31,10 +26,8 @@
[app.main.ui.releases :refer [release-notes-modal]]
[app.main.ui.static :as static]
[app.util.dom :as dom]
[app.util.i18n :refer [tr]]
[app.util.modules :as mod]
[app.util.theme :as theme]
[beicon.v2.core :as rx]
[rumext.v2 :as mf]))
(def auth-page
@ -55,79 +48,6 @@
(def workspace-page*
(mf/lazy #(mod/load 'app.main.ui.workspace/workspace-page*)))
(mf/defc workspace-legacy-redirect*
{::mf/props :obj
::mf/private true}
[{:keys [project-id file-id page-id layout]}]
(mf/with-effect []
(->> (rp/cmd! :get-project {:id project-id})
(rx/subs! (fn [{:keys [team-id]}]
(st/emit! (dcm/go-to-workspace :team-id team-id
:file-id file-id
:page-id page-id
:layout layout)))
errors/on-error)))
[:> loader*
{:title (tr "labels.loading")
:overlay true}])
(mf/defc dashboard-legacy-redirect*
{::mf/props :obj
::mf/private true}
[{:keys [section team-id project-id search-term plugin-url template]}]
(let [section (case section
:dashboard-legacy-search
:dashboard-search
:dashboard-legacy-projects
:dashboard-recent
:dashboard-legacy-files
:dashboard-files
:dashboard-legacy-libraries
:dashboard-libraries
:dashboard-legacy-fonts
:dashboard-fonts
:dashboard-legacy-font-providers
:dashboard-font-providers
:dashboard-legacy-team-members
:dashboard-members
:dashboard-legacy-team-invitations
:dashboard-invitations
:dashboard-legacy-team-webhooks
:dashboard-webhooks
:dashboard-legacy-team-settings
:dashboard-settings)]
(mf/with-effect []
(let [params {:team-id team-id
:project-id project-id
:search-term search-term
:plugin plugin-url
:template template}]
(st/emit! (rt/nav section (d/without-nils params)))))
[:> loader*
{:title (tr "labels.loading")
:overlay true}]))
(mf/defc viewer-legacy-redirect*
{::mf/props :obj
::mf/private true}
[{:keys [page-id file-id section index share-id interactions-mode frame-id share]}]
(mf/with-effect []
(let [params {:page-id page-id
:file-id file-id
:section section
:index index
:share-id share-id
:interactions-mode interactions-mode
:frame-id frame-id
:share share}]
(st/emit! (rt/nav :viewer (d/without-nils params)))))
[:> loader*
{:title (tr "labels.loading")
:overlay true}])
(mf/defc team-container*
{::mf/props :obj
::mf/private true}
@ -315,57 +235,6 @@
:share share}]])
:workspace-legacy
(let [project-id (some-> params :path :project-id uuid/parse*)
file-id (some-> params :path :file-id uuid/parse*)
page-id (some-> params :query :page-id uuid/parse*)
layout (some-> params :query :layout keyword)]
[:> workspace-legacy-redirect*
{:project-id project-id
:file-id file-id
:page-id page-id
:layout layout}])
(:dashboard-legacy-search
:dashboard-legacy-projects
:dashboard-legacy-files
:dashboard-legacy-libraries
:dashboard-legacy-fonts
:dashboard-legacy-font-providers
:dashboard-legacy-team-members
:dashboard-legacy-team-invitations
:dashboard-legacy-team-webhooks
:dashboard-legacy-team-settings)
(let [team-id (some-> params :path :team-id uuid/parse*)
project-id (some-> params :path :project-id uuid/parse*)
search-term (some-> params :query :search-term)
plugin-url (some-> params :query :plugin)
template (some-> params :template)]
[:> dashboard-legacy-redirect*
{:team-id team-id
:section section
:project-id project-id
:search-term search-term
:plugin-url plugin-url
:template template}])
:viewer-legacy
(let [{:keys [query-params path-params]} route
{:keys [index share-id section page-id interactions-mode frame-id share]
:or {section :interactions interactions-mode :show-on-click}} query-params
{:keys [file-id]} path-params]
[:> viewer-legacy-redirect*
{:page-id page-id
:file-id file-id
:section section
:index index
:share-id share-id
:interactions-mode (keyword interactions-mode)
:frame-id frame-id
:share share}])
:frame-preview
[:> frame-preview/frame-preview*]

17
frontend/src/app/main/ui/routes.cljs
View File

@ -55,8 +55,6 @@
["/view" :viewer]
["/view/:file-id" :viewer-legacy]
(when *assert*
["/debug/icons-preview" :debug-icons-preview])
@ -79,20 +77,7 @@
["/files" :dashboard-files]
["/deleted" :dashboard-deleted]]
["/dashboard/team/:team-id"
["/members" :dashboard-legacy-team-members]
["/invitations" :dashboard-legacy-team-invitations]
["/webhooks" :dashboard-legacy-team-webhooks]
["/settings" :dashboard-legacy-team-settings]
["/projects" :dashboard-legacy-projects]
["/search" :dashboard-legacy-search]
["/fonts" :dashboard-legacy-fonts]
["/fonts/providers" :dashboard-legacy-font-providers]
["/libraries" :dashboard-legacy-libraries]
["/projects/:project-id" :dashboard-legacy-files]]
["/workspace" :workspace]
["/workspace/:project-id/:file-id" :workspace-legacy]])
["/workspace" :workspace]])
(defn- store-session-params

58
frontend/test/frontend_tests/router_test.cljs
View File

@ -6,6 +6,8 @@
(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]
@ -19,13 +21,6 @@
(t/is (= {:file-id "file-1"}
(rt/match->context-params match)))))
(t/deftest match-context-params-file-link-path-params
;; Legacy routes carry the ids as path params.
(let [match {:params {:path {:project-id "project-1"
:file-id "file-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"}}]
@ -91,14 +86,34 @@
(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)))
(t/deftest navigated-mirrors-context-on-change
;; New context in the match triggers exactly one mirrored write.
;; 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/"}
calls
(fn []
(ptk/effect (rt/navigated {:query-params {:file-id "file-1"}} false) nil nil)
(emit-navigated {:query-params {:file-id "file-1"}} "http://localhost/")
(t/is (= ["/?file-id=file-1#/workspace?file-id=file-1"] @calls))))))
(t/deftest navigated-skips-write-when-mirrored
@ -108,7 +123,7 @@
{:pathname "/" :search "?file-id=file-1" :hash "#/workspace?file-id=file-1" :href "http://localhost/?file-id=file-1#/workspace?file-id=file-1"}
calls
(fn []
(ptk/effect (rt/navigated {:query-params {:file-id "file-1"}} false) nil nil)
(emit-navigated {:query-params {:file-id "file-1"}} "http://localhost/")
(t/is (= [] @calls))))))
(t/deftest navigated-strips-stale-context
@ -118,5 +133,26 @@
{:pathname "/" :search "?file-id=old" :hash "#/dashboard/recent?team-id=team-1" :href "http://localhost/?file-id=old#/dashboard/recent?team-id=team-1"}
calls
(fn []
(ptk/effect (rt/navigated {:query-params {:team-id "team-1"}} false) nil nil)
(emit-navigated {:query-params {:team-id "team-1"}} "http://localhost/")
(t/is (= ["/?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"}
calls
(fn []
(emit-navigated {:query-params {:token "some-token"}} "http://localhost/")
(t/is (= ["/#/auth/login"] @calls))))))
(t/deftest navigated-keeps-subpath-base
;; Under a subpath deployment the mirrored href keeps the prefix
;; from cf/public-uri.
(let [calls (atom [])]
(with-stubbed-browser
{:pathname "/penpot/" :search "" :hash "#/workspace?file-id=file-1" :href "http://localhost/penpot/"}
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))))))