From 90d98b1989aea78d838c090dac8948d26629dd6c Mon Sep 17 00:00:00 2001 From: Filip Sajdak Date: Tue, 10 Nov 2026 11:32:58 +0100 Subject: [PATCH] :bug: Call the team hook on every render of the team container (#12920) team-container* called mf/deref only after returning early for a team id that is not a uuid. When the same component rendered once without a team id and then with one, the number of hooks changed between the two renders and React threw "Rendered more hooks than during the previous render" (error #310), which sent the user to the internal error page. The reverse order throws "Rendered fewer hooks than expected". Call the hook on every render and keep the uuid check in the condition that builds the providers. Two tests mount the component in jsdom and render it with a team id that arrives and with one that leaves; both fail without this change. Closes #12728 AI-assisted-by: claude-opus-5-5 AI-assisted-by: claude-sonnet-5-5 Signed-off-by: Filip Sajdak --- frontend/src/app/main/ui.cljs | 18 ++--- frontend/test/frontend_tests/runner.cljs | 2 + .../ui/team_container_test.cljs | 80 +++++++++++++++++++ 3 files changed, 90 insertions(+), 10 deletions(-) create mode 100644 frontend/test/frontend_tests/ui/team_container_test.cljs diff --git a/frontend/src/app/main/ui.cljs b/frontend/src/app/main/ui.cljs index 5e2075082c..13c077c99c 100644 --- a/frontend/src/app/main/ui.cljs +++ b/frontend/src/app/main/ui.cljs @@ -64,16 +64,14 @@ :code :invalid-team-id :hint "provided team-id is invalid or nil"))) - (if-not (uuid? team-id) - nil - (let [{:keys [permissions] :as team} (mf/deref refs/team)] - (when (= team-id (:id team)) - [:> (mf/provider ctx/current-team-id) {:value team-id} - [:> (mf/provider ctx/permissions) {:value permissions} - [:> (mf/provider ctx/can-edit?) {:value (:can-edit permissions)} - ;; The `:key` is mandatory here because we want to reinitialize - ;; all dom tree instead of simple rerender. - [:* {:key (str team-id)} children]]]])))) + (let [{:keys [permissions] :as team} (mf/deref refs/team)] + (when (and (uuid? team-id) (= team-id (:id team))) + [:> (mf/provider ctx/current-team-id) {:value team-id} + [:> (mf/provider ctx/permissions) {:value permissions} + [:> (mf/provider ctx/can-edit?) {:value (:can-edit permissions)} + ;; The `:key` is mandatory here because we want to reinitialize + ;; all dom tree instead of simple rerender. + [:* {:key (str team-id)} children]]]]))) (mf/defc page* {::mf/props :obj diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 0e6187caff..26adc6eca4 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -167,6 +167,7 @@ [frontend-tests.ui.sortable-auto-scroll-test] [frontend-tests.ui.static-test] [frontend-tests.ui.stroke-menu-test] + [frontend-tests.ui.team-container-test] [frontend-tests.ui.text-attrs-multiple-test] [frontend-tests.ui.viewport-top-bar-test] [frontend-tests.ui.workspace-libraries-test] @@ -369,6 +370,7 @@ 'frontend-tests.ui.sortable-auto-scroll-test 'frontend-tests.ui.static-test 'frontend-tests.ui.stroke-menu-test + 'frontend-tests.ui.team-container-test 'frontend-tests.ui.text-attrs-multiple-test 'frontend-tests.ui.viewport-top-bar-test 'frontend-tests.ui.workspace-libraries-test diff --git a/frontend/test/frontend_tests/ui/team_container_test.cljs b/frontend/test/frontend_tests/ui/team_container_test.cljs new file mode 100644 index 0000000000..b16d0b089d --- /dev/null +++ b/frontend/test/frontend_tests/ui/team_container_test.cljs @@ -0,0 +1,80 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.ui.team-container-test + "`team-container*` renders the same hooks with and without a team id. + React throws when the hook count changes between two renders of the + same component, so these tests mount it and render it again; the + documents come from jsdom since the runner has no DOM." + (:require + ["jsdom" :refer [JSDOM]] + ["react-dom" :as rd] + ["react-dom/client" :as rdc] + [app.common.uuid :as uuid] + [app.main.refs :as refs] + [app.main.ui :as ui] + [cljs.test :as t :include-macros true] + [cuerdas.core :as str] + [okulary.core :as l] + [rumext.v2 :as mf])) + +(def ^:private team-id (uuid/next)) + +(defn- next-immediate + "Resolves after the tasks React's scheduler queued with `setImmediate` + so far have run." + [] + (js/Promise. (fn [resolve] (js/setImmediate resolve)))) + +(defn- ^:async render-with + "Mounts `team-container*` once and renders it again with each of + `team-ids`, in order. Resolves to the errors React reported and the + markup left in the document. + + react-dom reads the global `window` while it renders, and its scheduler + does so again on a later turn, so the jsdom one stays installed until + the root is unmounted and the scheduler has run. + + The effect is replaced by a hook that does nothing: the real one raises + for an invalid id, and what is under test is the render. It must still + take a hook slot, because React only compares the hooks of two renders + when the first one had any." + [team-ids] + (let [errors (volatile! []) + on-error (fn [error _info] (vswap! errors conj error)) + window (.-window (JSDOM. "
")) + container (.getElementById (.-document window) "root") + root (rdc/createRoot container #js {:onUncaughtError on-error + :onCaughtError on-error + :onRecoverableError on-error}) + child (mf/html [:span "inside"]) + previous (.-window js/globalThis)] + (set! (.-window js/globalThis) window) + (try + (with-redefs [refs/team (l/atom {:id team-id :permissions {:can-edit true}}) + mf/useEffect (fn [_f _deps] (mf/use-ref nil))] + (doseq [id team-ids] + (rd/flushSync + (fn [] + (.render root (mf/element (deref #'ui/team-container*) + #js {:teamId id :children child})))))) + (let [markup (.-innerHTML container)] + (rd/flushSync #(.unmount root)) + (await (next-immediate)) + {:errors @errors + :markup markup}) + (finally + (set! (.-window js/globalThis) previous))))) + +(t/deftest ^:async team-id-arriving-after-the-first-render-keeps-the-hooks + (let [{:keys [errors markup]} (await (render-with [nil team-id]))] + (t/is (empty? (map #(.-message %) errors))) + (t/is (str/includes? markup "inside")))) + +(t/deftest ^:async team-id-leaving-after-the-first-render-keeps-the-hooks + (let [{:keys [errors markup]} (await (render-with [team-id nil]))] + (t/is (empty? (map #(.-message %) errors))) + (t/is (= "" markup))))