From 80fc70c211a800e15b9d04faccf43be9bda847ef Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 9 Sep 2026 18:10:56 +0000 Subject: [PATCH] :recycle: Simplify router context mirroring and remove legacy routes 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 --- .../developer/subsystems/link-preview.md | 27 ++-- frontend/src/app/main/router.cljs | 41 ++++-- frontend/src/app/main/ui.cljs | 131 ------------------ frontend/src/app/main/ui/routes.cljs | 17 +-- frontend/test/frontend_tests/router_test.cljs | 58 ++++++-- 5 files changed, 91 insertions(+), 183 deletions(-) diff --git a/docs/technical-guide/developer/subsystems/link-preview.md b/docs/technical-guide/developer/subsystems/link-preview.md index 2bfd75ce43..52beba5c58 100644 --- a/docs/technical-guide/developer/subsystems/link-preview.md +++ b/docs/technical-guide/developer/subsystems/link-preview.md @@ -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=#/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 diff --git a/frontend/src/app/main/router.cljs b/frontend/src/app/main/router.cljs index 5fd748f51b..890163fb75 100644 --- a/frontend/src/app/main/router.cljs +++ b/frontend/src/app/main/router.cljs @@ -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))] diff --git a/frontend/src/app/main/ui.cljs b/frontend/src/app/main/ui.cljs index cb7fe8c87a..2286652cff 100644 --- a/frontend/src/app/main/ui.cljs +++ b/frontend/src/app/main/ui.cljs @@ -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*] diff --git a/frontend/src/app/main/ui/routes.cljs b/frontend/src/app/main/ui/routes.cljs index 6502958f1e..27f4ed8812 100644 --- a/frontend/src/app/main/ui/routes.cljs +++ b/frontend/src/app/main/ui/routes.cljs @@ -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 diff --git a/frontend/test/frontend_tests/router_test.cljs b/frontend/test/frontend_tests/router_test.cljs index e72f3a49e4..f490d67bcd 100644 --- a/frontend/test/frontend_tests/router_test.cljs +++ b/frontend/test/frontend_tests/router_test.cljs @@ -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))))))