From d669eec0c014bc9bb71896d26e286978af90648a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero-Cantero?= <807608+alvorithm@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:50:58 +0200 Subject: [PATCH] :sparkles: Load the branch list only when the switcher opens (#12571) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :sparkles: Add a branch count query for the workspace header The header shows its branch row only when the file has branches, and it learnt that from get-file-branches, which compares every diverged branch against main. get-file-branch-count answers the same question with one indexed count: archived and merged branches included, counted from main when the file is itself a branch, with no comparison and no climit, like the branches_count subquery of the dashboard listings. The listing keeps its climit, and :get-file-branches/global now sets the 60 s timeout that the header of climit.edn promises, so a waiter behind six cold listings gets an error instead of waiting without bound. Its rationale comment names the callers it serves now. AI-assisted-by: claude-opus-5-5 Signed-off-by: Álvaro Tejero Cantero * :sparkles: List branches only when the switcher opens Opening a file ran the counted branch listing for every user, because the header decided whether to show its branch row from that listing. A cold listing runs a three-way comparison per diverged branch, and one save to main makes every branch of the file a cache miss again. The header now loads the branch count and the branch context with the file. The listing loads when the branch switcher or the all-branches dialog opens, and both show "Loading branches…" until it arrives instead of a list from an earlier opening. A failed listing used to read as an empty list, which on main hid the branch row and sent the shortcut to the create dialog. It now ends in an error state: the switcher and the dialog say "Couldn't load branches" with a Retry action, and the row stays, since its visibility comes from the count. A successful listing refreshes the count, so deleting the last branch from the list still hides the row. Creating a pull request from the branch banner looked the branch up in the listing to decide whether to save pending edits first. Without the listing loaded at open it would skip the save, so it now reads the branch context of the open file. AI-assisted-by: claude-opus-5-5 Signed-off-by: Álvaro Tejero Cantero * :sparkles: Show the branch row when the branch count fails A failed get-file-branch-count left no count, so on main the header hid its branch row and the shortcut opened the create dialog on a file that may have branches. The failure is now kept as an error state under :workspace-branch-count, and has-branches? reads it as true. The row then shows the Main chip and, where the status note goes, "Couldn't load branches" in the warning colour. Opening the switcher loads the listing through the existing loading and Retry path, and a successful listing settles the count, which replaces the note. AI-assisted-by: claude-opus-5-5 Signed-off-by: Álvaro Tejero Cantero * :books: Name the callers of the branch listing limit again The merge of iop-vc-preview took its climit.edn, whose comment on :get-file-branches/global still described a panel open. Restore the comment this branch wrote: the listing runs when the switcher, the all-branches dialog or the dashboard popover opens, and a file open only counts branches. AI-assisted-by: claude-opus-5-5 Signed-off-by: Álvaro Tejero Cantero * :bug: Mock the branch count in the library copy tests The header now shows its branch row from get-file-branch-count, not from the listing, so the library copy tests mock the count as well. AI-assisted-by: claude-opus-5-5 Signed-off-by: Álvaro Tejero Cantero --------- Signed-off-by: Álvaro Tejero Cantero --- backend/resources/climit.edn | 7 +- backend/src/app/rpc/commands/files_branch.clj | 36 +++++ .../backend_tests/rpc_file_branch_test.clj | 49 ++++++ .../branches/get-file-branch-count-two.json | 3 + .../data/branches/get-file-branches-two.json | 32 ++++ .../ui/specs/branch-switcher.spec.js | 151 ++++++++++++++++++ .../ui/specs/branches-library.spec.js | 4 + .../src/app/main/data/workspace/branches.cljs | 59 ++++++- .../main/data/workspace/pull_requests.cljs | 9 +- .../main/ui/workspace/branch_switcher.cljs | 109 +++++++++---- .../main/ui/workspace/branch_switcher.scss | 10 ++ .../main/ui/workspace/sidebar/branches.cljs | 30 +++- .../data/workspace_branches_test.cljs | 122 +++++++++++++- frontend/translations/en.po | 8 +- 14 files changed, 579 insertions(+), 50 deletions(-) create mode 100644 frontend/playwright/data/branches/get-file-branch-count-two.json create mode 100644 frontend/playwright/data/branches/get-file-branches-two.json create mode 100644 frontend/playwright/ui/specs/branch-switcher.spec.js diff --git a/backend/resources/climit.edn b/backend/resources/climit.edn index 660d2abca6..1d10df0f8f 100644 --- a/backend/resources/climit.edn +++ b/backend/resources/climit.edn @@ -169,8 +169,11 @@ {:permits 1 :queue 2 :timeout 60000} ;; A cold listing realizes main once and derives every uncached branch, - ;; which makes it the slowest read of the preview. Six at once is above - ;; what a panel open needs under normal use, and six three-document + ;; which makes it the slowest read of the preview. It runs when a user + ;; opens the workspace branch switcher, the all-branches dialog, or the + ;; dashboard branches popover; a file open only counts branches, through + ;; `get-file-branch-count`, which takes no limit. Six at once is above + ;; what those opens need under normal use, and six three-document ;; operations hold eighteen realized documents, below the twenty ;; one-document saves `:update-file/global` above already admits. ;; Provisional. diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index d48d3cdc5e..5adcfa7e4b 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -1774,6 +1774,42 @@ (assoc :ahead ahead :behind behind :conflicts conflicts) (dissoc :branch-revn :source-revn :base-snapshot-id :base-branch-revn)))))))) +;; --- COMMAND QUERY: get-file-branch-count + +(def ^:private schema:get-file-branch-count + [:map {:title "get-file-branch-count"} + [:file-id ::sm/uuid]]) + +(def ^:private sql:get-file-branch-count + "SELECT count(*) AS count + FROM file_branch AS fb + WHERE fb.deleted_at IS NULL + AND fb.source_file_id = coalesce((SELECT b.source_file_id + FROM file_branch AS b + WHERE b.branch_file_id = ? + AND b.deleted_at IS NULL), + ?)") + +(sv/defmethod ::get-file-branch-count + "How many branches the file has, archived and merged ones included, as + the listing with `:include-archived` returns them. When the file is + itself a branch, the branches of its source (main) are counted. + + The workspace header asks this on every file open to decide whether to + show its branch row, so it compares nothing and takes no climit: it is + the dashboard's `branches_count` subquery, one indexed count. Returns a + zero count when branching is disabled, so it is safe on every open." + {::doc/added "2.16" + ::sm/params schema:get-file-branch-count} + [cfg {:keys [::rpc/profile-id file-id]}] + (if (contains? cf/flags :branching) + (db/run! cfg + (fn [{:keys [::db/conn] :as cfg}] + (files/check-read-permissions! cfg profile-id file-id) + (-> (db/exec-one! conn [sql:get-file-branch-count file-id file-id]) + (select-keys [:count])))) + {:count 0})) + ;; --- COMMAND: update-file-branch (rename / description) (def ^:private schema:update-file-branch diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index fccb45e45d..bc01053a30 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -618,6 +618,55 @@ color (get-in out [:result :data :colors cid])] (t/is (= "#00ff00" (:color color))))))))) +(t/deftest branch-count-counts-from-main-without-comparing + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [profile (th/create-profile* 1 {:is-active true}) + stranger (th/create-profile* 2 {:is-active true}) + proj-id (:default-project-id profile) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id proj-id + :is-shared false}) + create! (fn [name] + (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name name}))) + count-of (fn [profile-id file-id] + ;; the count answers on every file open: it must never + ;; reach the comparison the listing pays for + (with-redefs [fbranch/cached-diff-counts + (fn [& _] (throw (ex-info "compared" {})))] + (th/command! {::th/type :get-file-branch-count + ::rpc/profile-id profile-id + :file-id file-id})))] + + (t/testing "a file with no branches counts zero" + (let [out (count-of (:id profile) (:id file))] + (t/is (nil? (:error out))) + (t/is (= {:count 0} (:result out))))) + + (let [kept (create! "kept") + archived (create! "archived") + deleted (create! "deleted")] + (th/command! {::th/type :archive-file-branch + ::rpc/profile-id (:id profile) + :id (:id archived)}) + (th/command! {::th/type :delete-file-branch + ::rpc/profile-id (:id profile) + :id (:id deleted)}) + + (t/testing "archived branches count, deleted ones do not" + (t/is (= {:count 2} (:result (count-of (:id profile) (:id file)))))) + + (t/testing "from a branch, the branches of its main are counted" + (t/is (= {:count 2} (:result (count-of (:id profile) (:branch-file-id kept)))))) + + (t/testing "a profile that cannot read the file is refused" + (let [out (count-of (:id stranger) (:id file))] + (t/is (= :not-found (-> out :error ex-data :type)))) + (let [out (count-of (:id stranger) (:branch-file-id kept))] + (t/is (= :not-found (-> out :error ex-data :type))))))))) + (t/deftest branching-disabled-raises (let [profile (th/create-profile* 1 {:is-active true}) proj-id (:default-project-id profile) diff --git a/frontend/playwright/data/branches/get-file-branch-count-two.json b/frontend/playwright/data/branches/get-file-branch-count-two.json new file mode 100644 index 0000000000..5904151986 --- /dev/null +++ b/frontend/playwright/data/branches/get-file-branch-count-two.json @@ -0,0 +1,3 @@ +{ + "~:count": 2 +} diff --git a/frontend/playwright/data/branches/get-file-branches-two.json b/frontend/playwright/data/branches/get-file-branches-two.json new file mode 100644 index 0000000000..8504ec9aed --- /dev/null +++ b/frontend/playwright/data/branches/get-file-branches-two.json @@ -0,0 +1,32 @@ +[ + { + "~:id": "~uc7ce0794-0992-8105-8004-38f280443a01", + "~:branch-file-id": "~uc7ce0794-0992-8105-8004-38f280443b01", + "~:source-file-id": "~uc7ce0794-0992-8105-8004-38f280443997", + "~:name": "checkout", + "~:description": null, + "~:status": "open", + "~:created-by": "~uc7ce0794-0992-8105-8004-38e630f29a9b", + "~:created-at": "~m1713536343369", + "~:updated-at": "~m1713873823633", + "~:source-name": "New File 1", + "~:ahead": 1, + "~:behind": 0, + "~:conflicts": 0 + }, + { + "~:id": "~uc7ce0794-0992-8105-8004-38f280443a02", + "~:branch-file-id": "~uc7ce0794-0992-8105-8004-38f280443b02", + "~:source-file-id": "~uc7ce0794-0992-8105-8004-38f280443997", + "~:name": "old-header", + "~:description": null, + "~:status": "archived", + "~:created-by": "~uc7ce0794-0992-8105-8004-38e630f29a9b", + "~:created-at": "~m1713536343369", + "~:updated-at": "~m1713873823633", + "~:source-name": "New File 1", + "~:ahead": 0, + "~:behind": 0, + "~:conflicts": 0 + } +] diff --git a/frontend/playwright/ui/specs/branch-switcher.spec.js b/frontend/playwright/ui/specs/branch-switcher.spec.js new file mode 100644 index 0000000000..7f4c6e2657 --- /dev/null +++ b/frontend/playwright/ui/specs/branch-switcher.spec.js @@ -0,0 +1,151 @@ +import { test, expect } from "@playwright/test"; +import { WorkspacePage } from "../pages/WorkspacePage"; + +const LISTING = "**/api/main/methods/get-file-branches?file-id=*"; + +test.beforeEach(async ({ page }) => { + await WorkspacePage.init(page); + await WorkspacePage.mockConfigFlags(page, ["enable-branching"]); +}); + +// main, with two branches (one of them archived) +const setupMainWithBranches = async (workspacePage) => { + await workspacePage.setupEmptyFile(); + await workspacePage.mockRPCs({ + "get-file-branch-info?file-id=*": + "pull-requests/get-file-branch-info-none.json", + "get-file-branch-count?file-id=*": + "branches/get-file-branch-count-two.json", + "get-file-branches?file-id=*": "branches/get-file-branches-two.json", + "get-file-pull-requests?file-id=*": + "pull-requests/get-file-branches-empty.json", + }); +}; + +// every listing request fails until `heal` is called +const failListing = async (page) => { + let failing = true; + await page.route(LISTING, (route) => + failing + ? route.fulfill({ + status: 500, + contentType: "application/transit+json", + body: JSON.stringify({ "~:type": "~:internal" }), + }) + : route.fallback(), + ); + return () => { + failing = false; + }; +}; + +test("opening a file counts branches without listing them", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupMainWithBranches(workspacePage); + const listings = []; + page.on("request", (request) => { + if (request.url().includes("/get-file-branches?")) listings.push(request); + }); + + await workspacePage.goToWorkspace(); + + // the count alone shows the row + const chip = page.getByTitle("Switch branch"); + await expect(chip).toBeVisible(); + expect(listings).toHaveLength(0); + + // the listing loads when the switcher opens + await chip.click(); + await expect(page.getByText("checkout", { exact: true })).toBeVisible(); + expect(listings).toHaveLength(1); +}); + +test("the switcher shows a loading state while the listing loads", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupMainWithBranches(workspacePage); + let release; + const released = new Promise((resolve) => { + release = resolve; + }); + await page.route(LISTING, async (route) => { + await released; + await route.fallback(); + }); + await workspacePage.goToWorkspace(); + + await page.getByTitle("Switch branch").click(); + await expect(page.getByText("Loading branches…")).toBeVisible(); + await expect(page.getByText("checkout", { exact: true })).toBeHidden(); + + release(); + await expect(page.getByText("checkout", { exact: true })).toBeVisible(); + await expect(page.getByText("Loading branches…")).toBeHidden(); +}); + +test("a failed listing in the switcher offers a retry and keeps the row", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupMainWithBranches(workspacePage); + const heal = await failListing(page); + await workspacePage.goToWorkspace(); + + const chip = page.getByTitle("Switch branch"); + await chip.click(); + await expect(page.getByText("Couldn't load branches")).toBeVisible(); + await expect(chip).toBeVisible(); + + heal(); + await page.getByRole("button", { name: "Retry" }).click(); + await expect(page.getByText("checkout", { exact: true })).toBeVisible(); + await expect(page.getByText("Couldn't load branches")).toBeHidden(); +}); + +test("a failed listing in the all-branches dialog offers a retry", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupMainWithBranches(workspacePage); + const heal = await failListing(page); + await workspacePage.goToWorkspace(); + + await page.getByTitle("Switch branch").click(); + await page.getByText("All branches").click(); + const dialog = page.getByRole("dialog"); + await expect(dialog.getByText("Couldn't load branches")).toBeVisible(); + + heal(); + await dialog.getByRole("button", { name: "Retry" }).click(); + await expect(dialog.getByText("checkout", { exact: true })).toBeVisible(); +}); + +test("a failed count shows the row with an error, and the switcher lists", async ({ + page, +}) => { + const workspacePage = new WorkspacePage(page); + await setupMainWithBranches(workspacePage); + await page.route( + "**/api/main/methods/get-file-branch-count?file-id=*", + (route) => + route.fulfill({ + status: 500, + contentType: "application/transit+json", + body: JSON.stringify({ "~:type": "~:internal" }), + }), + ); + await workspacePage.goToWorkspace(); + + // an unknown count never hides the row: the note says what failed + const chip = page.getByTitle("Switch branch"); + await expect(chip).toBeVisible(); + await expect(page.getByText("Couldn't load branches")).toBeVisible(); + + // opening the switcher loads the listing, which settles the count + await chip.click(); + await expect(page.getByText("checkout", { exact: true })).toBeVisible(); + await expect(page.getByText("Couldn't load branches")).toBeHidden(); +}); diff --git a/frontend/playwright/ui/specs/branches-library.spec.js b/frontend/playwright/ui/specs/branches-library.spec.js index ac135712e6..1d9c9a1354 100644 --- a/frontend/playwright/ui/specs/branches-library.spec.js +++ b/frontend/playwright/ui/specs/branches-library.spec.js @@ -44,6 +44,8 @@ test("on a library, the create dialog says consumers read main", async ({ }) => { const workspacePage = new WorkspacePage(page); await setupBranching(workspacePage, { + "get-file-branch-count?file-id=*": + "branches/get-file-branch-count-two.json", "get-file-branches?file-id=*": "pull-requests/get-file-branches-one.json", }); await workspacePage.mockGetFile("pull-requests/get-file-blank-shared.json"); @@ -67,6 +69,8 @@ test("on an ordinary file, the create dialog keeps its note", async ({ }) => { const workspacePage = new WorkspacePage(page); await setupBranching(workspacePage, { + "get-file-branch-count?file-id=*": + "branches/get-file-branch-count-two.json", "get-file-branches?file-id=*": "pull-requests/get-file-branches-one.json", }); await workspacePage.goToWorkspace(); diff --git a/frontend/src/app/main/data/workspace/branches.cljs b/frontend/src/app/main/data/workspace/branches.cljs index a311c58527..b501100e3f 100644 --- a/frontend/src/app/main/data/workspace/branches.cljs +++ b/frontend/src/app/main/data/workspace/branches.cljs @@ -73,7 +73,52 @@ (update [_ state] (update state :workspace-branches merge branch-state)))) +(defn- set-branch-count + "Store the branch count of `file-id`: `{:file-id .. :count n}`, or + `{:file-id .. :status :error}` when the count could not be loaded." + [file-id count-state] + (ptk/reify ::set-branch-count + ptk/UpdateEvent + (update [_ state] + (assoc state :workspace-branch-count (assoc count-state :file-id file-id))))) + +(defn fetch-branch-count + "Load how many branches the open file has, archived ones included and + counted from main when the file is itself a branch. The header reads it + to decide whether to show its branch row, so it loads with the file; it + compares nothing, unlike the listing, which loads only when a user opens + the switcher or the all-branches dialog. A count belonging to ANOTHER + file is dropped synchronously, as `fetch-branch-context` drops its own. + A failure is kept as an error, which shows the row: a count that is + unknown must not hide branches the file may have. The count lives under + its own key because `init-branches-state` resets `:workspace-branches`." + [] + (ptk/reify ::fetch-branch-count + ptk/UpdateEvent + (update [_ state] + (let [file-id (:current-file-id state)] + (if (= file-id (get-in state [:workspace-branch-count :file-id])) + state + (dissoc state :workspace-branch-count)))) + + ptk/WatchEvent + (watch [_ state _] + (when-let [file-id (:current-file-id state)] + (->> (rp/cmd! :get-file-branch-count {:file-id file-id}) + (rx/map #(set-branch-count file-id {:count (:count %)})) + (rx/catch (fn [_] (rx/of (set-branch-count file-id {:status :error}))))))))) + +(defn branch-count-failed? + "True when the branch count of the open file could not be loaded." + [state] + (= :error (get-in state [:workspace-branch-count :status]))) + (defn fetch-branches + "Load the branch listing of the open file. A failure leaves the listing + in `:error`, never in an empty list, so it cannot read as a file with no + branches. A successful listing also refreshes the branch count, since it + covers the same branches, so the header follows a delete or a + materialize made from the listing." [] (ptk/reify ::fetch-branches ptk/WatchEvent @@ -86,9 +131,11 @@ (rx/mapcat (fn [info] (let [root (or (:source-file-id info) file-id)] (rp/cmd! :get-file-branches {:file-id root :include-archived true})))) - (rx/map #(update-branches-state {:status :loaded :data %})) + (rx/mapcat (fn [data] + (rx/of (update-branches-state {:status :loaded :data data}) + (set-branch-count file-id {:count (count data)})))) (rx/catch (fn [_] - (rx/of (update-branches-state {:status :loaded :data []}))))))))) + (rx/of (update-branches-state {:status :error}))))))))) (defn fetch-branching-limits "Load the size limits this instance refuses beyond, so the panel can @@ -284,10 +331,14 @@ (defn has-branches? "True when the open file takes part in branching: it is a branch, or it is main and has at least one branch (archived ones count, so they stay - reachable). The header only shows the branch switcher then." + reachable). The header only shows the branch switcher then. It reads the + branch count, never the listing, so a listing that fails or is still + loading cannot hide the switcher. A count that failed shows the + switcher too: opening it loads the listing, which settles the count." [state] (or (some? (get-in state [:workspace-branch-context :info])) - (boolean (seq (get-in state [:workspace-branches :data]))))) + (branch-count-failed? state) + (pos? (get-in state [:workspace-branch-count :count] 0)))) (defn open-branch "Navigate to the branch file as a normal workspace file. The branch diff --git a/frontend/src/app/main/data/workspace/pull_requests.cljs b/frontend/src/app/main/data/workspace/pull_requests.cljs index 46676699be..5bd2bd6d19 100644 --- a/frontend/src/app/main/data/workspace/pull_requests.cljs +++ b/frontend/src/app/main/data/workspace/pull_requests.cljs @@ -111,10 +111,11 @@ (ptk/reify ::create-pull-request ptk/WatchEvent (watch [_ state _] - (let [branch (->> (get-in state [:workspace-branches :data]) - (filter #(= branch-id (:id %))) - (first)) - from-ws? (= (:branch-file-id branch) (:current-file-id state)) + (let [;; the branch context is the open file's, and it is loaded + ;; with the file; the listing loads only with the switcher + ctx (:workspace-branch-context state) + from-ws? (and (= (:file-id ctx) (:current-file-id state)) + (= branch-id (get-in ctx [:info :id]))) wait (if from-ws? (dwp/wait-persisted) (rx/of :ready))] diff --git a/frontend/src/app/main/ui/workspace/branch_switcher.cljs b/frontend/src/app/main/ui/workspace/branch_switcher.cljs index 50a6de071a..f03faca948 100644 --- a/frontend/src/app/main/ui/workspace/branch_switcher.cljs +++ b/frontend/src/app/main/ui/workspace/branch_switcher.cljs @@ -40,6 +40,9 @@ its chip only show then." (l/derived dwb/has-branches? st/state)) +(def ^:private count-failed + (l/derived dwb/branch-count-failed? st/state)) + (def ^:private branches (l/derived :workspace-branches st/state)) @@ -106,7 +109,7 @@ (mf/defc switcher-popover* {::mf/private true} [{:keys [file current prs-by-branch on-close]}] - (let [{:keys [data]} (mf/deref branches) + (let [{:keys [data status]} (mf/deref branches) filter* (mf/use-state "") filter-v (deref filter*) @@ -164,7 +167,21 @@ (mf/deps on-close) (fn [_] (on-close) - (modal/show! :branches-list {})))] + (modal/show! :branches-list {}))) + + on-retry + (mf/use-fn + (fn [event] + ;; the button unmounts as the listing reloads, so the + ;; dropdown would take the click for an outside one and close + (dom/stop-propagation event) + (st/emit! (dwb/init-branches-state))))] + + ;; the listing compares every diverged branch, so it loads only when + ;; the popover opens; before paint, so a list from an earlier opening + ;; never shows in place of the loading state + (mf/with-layout-effect [] + (st/emit! (dwb/init-branches-state))) [:div {:class (stl/css :popover) :role "dialog" @@ -182,32 +199,52 @@ :pr (get prs-by-branch (:id current)) :current true}]] - (when (seq others) - [:* - [:div {:class (stl/css :section)} - [:span (tr "workspace.branches.section.others")] - [:span (dm/str (count others))]] - [:ul {:class (stl/css :rows)} - (for [row others] - [:> switcher-row* {:key (dm/str (:branch-file-id row)) - :row row - :pr (get prs-by-branch (:id row)) - :on-open on-open}])]]) + (case status + :loading + [:> text* {:as "div" + :typography t/body-small + :role "status" + :class (stl/css :list-note)} + (tr "workspace.branches.loading")] - (when (seq archived) + :error [:* - [:button {:class (stl/css :action) - :aria-expanded show-archived? - :on-click on-toggle-archived} - [:> i/icon* {:icon-id (if show-archived? i/arrow-down i/arrow-right) :size "s"}] - [:span {:class (stl/css :action-label)} (tr "workspace.branches.archived")] - [:span {:class (stl/css :action-aside)} (dm/str (count archived))]] - (when show-archived? - [:ul {:class (stl/css :rows)} - (for [row archived] - [:> switcher-row* {:key (dm/str (:branch-file-id row)) - :row row - :on-open on-open}])])]) + [:> text* {:as "div" + :typography t/body-small + :role "alert" + :class (stl/css :list-note)} + (tr "workspace.branches.list.error")] + [:button {:class (stl/css :action) :on-click on-retry} + [:> i/icon* {:icon-id i/reload :size "s"}] + [:span {:class (stl/css :action-label)} (tr "labels.retry")]]] + + [:* + (when (seq others) + [:* + [:div {:class (stl/css :section)} + [:span (tr "workspace.branches.section.others")] + [:span (dm/str (count others))]] + [:ul {:class (stl/css :rows)} + (for [row others] + [:> switcher-row* {:key (dm/str (:branch-file-id row)) + :row row + :pr (get prs-by-branch (:id row)) + :on-open on-open}])]]) + + (when (seq archived) + [:* + [:button {:class (stl/css :action) + :aria-expanded show-archived? + :on-click on-toggle-archived} + [:> i/icon* {:icon-id (if show-archived? i/arrow-down i/arrow-right) :size "s"}] + [:span {:class (stl/css :action-label)} (tr "workspace.branches.archived")] + [:span {:class (stl/css :action-aside)} (dm/str (count archived))]] + (when show-archived? + [:ul {:class (stl/css :rows)} + (for [row archived] + [:> switcher-row* {:key (dm/str (:branch-file-id row)) + :row row + :on-open on-open}])])])]) [:hr {:class (stl/css :divider)}] @@ -228,6 +265,7 @@ [{:keys [file]}] (let [file-id (:id file) visible? (mf/deref has-branches) + failed? (mf/deref count-failed) open? (mf/deref switcher-open) ctx (mf/deref refs/branch-context) profile (mf/deref refs/profile) @@ -263,12 +301,14 @@ on-close (mf/use-fn #(st/emit! (dwb/toggle-branch-switcher false)))] - ;; the list decides whether the row shows at all, so it loads with the - ;; file, and so does the branch context: during a review the bottom - ;; banner that also loads it is replaced by the review banner + ;; the header needs to know only whether the file has branches, so it + ;; loads their count with the file and leaves the listing, which + ;; compares every diverged branch, to the popover; the branch context + ;; loads here too: during a review the bottom banner that also loads + ;; it is replaced by the review banner (mf/with-effect [file-id] (when (contains? cf/flags :branching) - (st/emit! (dwb/init-branches-state) + (st/emit! (dwb/fetch-branch-count) (dwb/fetch-branch-context) (dwb/toggle-branch-switcher false)))) @@ -302,6 +342,15 @@ ;; file stands until there are changes to count (let [pr (get prs-by-branch (:id ctx))] (cond + ;; the row shows because the count is unknown: say so, and the + ;; switcher, once opened, loads the listing that settles it + (and (nil? ctx) ^boolean failed?) + [:> text* {:as "span" + :typography t/body-small + :role "alert" + :class (stl/css :status-note :status-error)} + (tr "workspace.branches.list.error")] + (nil? ctx) [:> text* {:as "span" :typography t/body-small diff --git a/frontend/src/app/main/ui/workspace/branch_switcher.scss b/frontend/src/app/main/ui/workspace/branch_switcher.scss index ae206cc70b..8f7a794cab 100644 --- a/frontend/src/app/main/ui/workspace/branch_switcher.scss +++ b/frontend/src/app/main/ui/workspace/branch_switcher.scss @@ -29,6 +29,11 @@ color: var(--color-foreground-secondary); } +// the warning colour the branch status uses when a comparison failed +.status-error { + color: var(--color-accent-warning); +} + // --- Popover .popover { @@ -66,6 +71,11 @@ list-style: none; } +.list-note { + padding: var(--sp-s); + color: var(--color-foreground-secondary); +} + .action { @include use-typography("body-small"); diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs index e3794e2189..8e50fcb745 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs @@ -897,7 +897,7 @@ {::mf/register modal/components ::mf/register-as :branches-list} [] - (let [{:keys [data]} (mf/deref branches) + (let [{:keys [data status]} (mf/deref branches) profiles (mf/deref refs/profiles) profile (mf/deref refs/profile) file (mf/deref refs/file) @@ -966,9 +966,14 @@ (mf/use-fn (mf/deps file) (fn [_] - (modal/show! :create-branch {:file-name (:name file)})))] + (modal/show! :create-branch {:file-name (:name file)}))) - (mf/with-effect [] + on-retry + (mf/use-fn #(st/emit! (dwb/init-branches-state)))] + + ;; before paint, so a list from an earlier opening never shows in + ;; place of the loading state + (mf/with-layout-effect [] (st/emit! (dwb/init-branches-state)) (when (dwpr/enabled?) (st/emit! (dwpr/fetch-pull-requests)))) @@ -985,10 +990,27 @@ :action-button-position "end" :action-button search :class (stl/css :branches-list-tabs)} - (if (empty? rows) + (cond + (= status :loading) + [:div {:class (stl/css :branches-empty) :role "status"} + [:> empty-state* {:icon i/git-branch + :text (tr "workspace.branches.loading")}]] + + (= status :error) + [:div {:class (stl/css :branches-empty) :role "alert"} + [:> empty-state* {:icon i/triangle-alert + :text (tr "workspace.branches.list.error")}] + [:> button* {:variant "secondary" + :icon i/reload + :on-click on-retry} + (tr "labels.retry")]] + + (empty? rows) [:div {:class (stl/css :branches-empty)} [:> empty-state* {:icon i/git-branch :text (tr "workspace.branches.list.empty")}]] + + :else [:ul {:class (stl/css :branches-entries :branches-list-entries)} (for [entry rows] (let [k (dm/str (:id entry))] diff --git a/frontend/test/frontend_tests/data/workspace_branches_test.cljs b/frontend/test/frontend_tests/data/workspace_branches_test.cljs index e864dada27..bedb5b630b 100644 --- a/frontend/test/frontend_tests/data/workspace_branches_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_branches_test.cljs @@ -10,7 +10,9 @@ [app.config :as cf] [app.main.data.common :as dcm] [app.main.data.modal :as modal] + [app.main.data.persistence :as dwp] [app.main.data.workspace.branches :as dwb] + [app.main.data.workspace.pull-requests :as dwpr] [app.main.repo :as rp] [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] @@ -233,12 +235,11 @@ (def ^:private main-without-branches {:current-file-id file-id :files {file-id {:id file-id :name "Checkout"}} - :workspace-branches {:status :loaded :data []} + :workspace-branch-count {:file-id file-id :count 0} :workspace-branch-context {:file-id file-id :loaded? true :info nil}}) (def ^:private main-with-branches - (assoc main-without-branches :workspace-branches - {:status :loaded :data [{:id (uuid/next) :name "feature-login" :status "open"}]})) + (assoc main-without-branches :workspace-branch-count {:file-id file-id :count 1})) (def ^:private on-a-branch (assoc main-without-branches :workspace-branch-context @@ -248,7 +249,120 @@ (t/is (false? (dwb/has-branches? main-without-branches))) (t/is (true? (dwb/has-branches? main-with-branches))) (t/is (true? (dwb/has-branches? on-a-branch)) - "a branch counts even before the list loads")) + "a branch counts even before the count loads")) + +(t/deftest the-listing-never-decides-whether-the-switcher-exists + (t/is (true? (dwb/has-branches? (assoc main-with-branches :workspace-branches {:status :error}))) + "a failed listing keeps the switcher") + (t/is (true? (dwb/has-branches? (assoc main-with-branches :workspace-branches {:status :loading}))) + "a loading listing keeps the switcher") + (t/is (false? (dwb/has-branches? (assoc main-without-branches :workspace-branches + {:status :loaded :data [{:id (uuid/next)}]}))) + "the count decides, not a listing left over from another file")) + +(t/deftest ^:async a-failed-listing-is-an-error-not-an-empty-list + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [_ _] (rx/throw (ex-info "network" {}))))} + (let [store (make-store-with main-with-branches)] + (ptk/emit! store (dwb/init-branches-state)) + (await (async/wait-for #(not= :loading (get-in @store [:workspace-branches :status])) + "the listing settles")) + (t/is (= :error (get-in @store [:workspace-branches :status]))) + (t/is (nil? (get-in @store [:workspace-branches :data]))) + (t/is (true? (dwb/has-branches? @store)) "the switcher stays") + (rx/dispose! store))))) + +(t/deftest ^:async the-count-loads-without-the-listing + (let [calls (atom [])] + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [cmd params] + (swap! calls conj [cmd params]) + (rx/of {:count 3})))} + (let [store (make-store-with (dissoc main-without-branches :workspace-branch-count))] + (ptk/emit! store (dwb/fetch-branch-count)) + (await (async/wait-for #(some? (:workspace-branch-count @store)) "the count lands")) + (t/is (= {:file-id file-id :count 3} (:workspace-branch-count @store))) + (t/is (= [[:get-file-branch-count {:file-id file-id}]] @calls) + "only the count is asked, never the listing") + (t/is (true? (dwb/has-branches? @store))) + (rx/dispose! store)))))) + +(t/deftest ^:async the-count-of-another-file-is-dropped-at-once + (let [other (uuid/next)] + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [_ _] (rx/subject)))} + (let [store (make-store-with (assoc main-with-branches :workspace-branch-count + {:file-id other :count 4}))] + (ptk/emit! store (dwb/fetch-branch-count)) + (t/is (nil? (:workspace-branch-count @store))) + (t/is (false? (dwb/has-branches? @store))) + (rx/dispose! store)))))) + +(t/deftest ^:async a-failed-count-shows-the-switcher-with-an-error + (await (mock/with-mocks* + {cf/flags (conj cf/flags :branching) + rp/cmd! (mock/stub (fn [_ _] (rx/throw (ex-info "network" {}))))} + (let [store (make-store-with (dissoc main-without-branches :workspace-branch-count))] + (ptk/emit! store (dwb/fetch-branch-count)) + (await (async/wait-for #(some? (:workspace-branch-count @store)) "the count settles")) + (t/is (= {:file-id file-id :status :error} (:workspace-branch-count @store))) + (t/is (true? (dwb/branch-count-failed? @store))) + (t/is (true? (dwb/has-branches? @store)) + "a count that failed never hides the switcher") + (ptk/emit! store (dwb/show-branches)) + (await (async/wait-for #(true? (:workspace-branch-switcher @store)) + "the shortcut opens the switcher, which retries")) + (t/is (nil? (::modal/modal @store)) "no create dialog on an unknown count") + (rx/dispose! store))))) + +(t/deftest ^:async a-listing-after-a-failed-count-settles-it + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [cmd _] + (case cmd + :get-file-branch-info (rx/of nil) + :get-file-branches (rx/of [{:id (uuid/next) :status "open"}]) + (rx/empty))))} + (let [store (make-store-with (assoc main-without-branches :workspace-branch-count + {:file-id file-id :status :error}))] + (ptk/emit! store (dwb/init-branches-state)) + (await (async/wait-for #(= :loaded (get-in @store [:workspace-branches :status])) + "the listing lands")) + (t/is (= {:file-id file-id :count 1} (:workspace-branch-count @store))) + (t/is (false? (dwb/branch-count-failed? @store))) + (rx/dispose! store))))) + +(t/deftest ^:async a-listing-refreshes-the-count + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [cmd _] + (case cmd + :get-file-branch-info (rx/of nil) + :get-file-branches (rx/of []) + (rx/empty))))} + (let [store (make-store-with main-with-branches)] + (ptk/emit! store (dwb/fetch-branches)) + (await (async/wait-for #(= :loaded (get-in @store [:workspace-branches :status])) + "the listing lands")) + (t/is (= 0 (get-in @store [:workspace-branch-count :count])) + "deleting the last branch from the listing hides the switcher") + (t/is (false? (dwb/has-branches? @store))) + (rx/dispose! store))))) + +(t/deftest ^:async a-pull-request-from-the-open-branch-saves-first-without-the-listing + (let [waits (atom 0) + branch-id (uuid/next)] + (await (mock/with-mocks* + {dwp/wait-persisted (mock/stub (fn [& _] (swap! waits inc) (rx/of :saved))) + rp/cmd! (mock/stub (fn [_ _] (rx/empty)))} + ;; the bottom banner opens the dialog with the branch context; + ;; the listing only loads with the switcher, so it is absent + (let [store (make-store-with + (assoc main-without-branches :workspace-branch-context + {:file-id file-id + :loaded? true + :info {:id branch-id :branch-file-id file-id}}))] + (ptk/emit! store (dwpr/create-pull-request branch-id {:title "Review me"})) + (await (async/wait-for #(pos? @waits) "the pending edits are saved first")) + (rx/dispose! store)))))) (t/deftest the-switcher-toggles-and-can-be-forced (let [store (make-store-with main-with-branches)] diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 40f8285671..a5b4bc6982 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -3292,7 +3292,7 @@ msgstr "Resend invitation" msgid "labels.restore" msgstr "Restore" -#: src/app/main/ui/components/progress.cljs:80, src/app/main/ui/static.cljs:333, src/app/main/ui/static.cljs:342, src/app/main/ui/static.cljs:437 +#: src/app/main/ui/components/progress.cljs:80, src/app/main/ui/static.cljs:333, src/app/main/ui/static.cljs:342, src/app/main/ui/static.cljs:437, src/app/main/ui/workspace/branch_switcher.cljs:210, src/app/main/ui/workspace/sidebar/branches.cljs:991 msgid "labels.retry" msgstr "Retry" @@ -7120,6 +7120,10 @@ msgstr "A branch name can't be empty." msgid "workspace.branches.list.empty" msgstr "No branches here" +#: src/app/main/ui/workspace/branch_switcher.cljs:207, src/app/main/ui/workspace/branch_switcher.cljs:334, src/app/main/ui/workspace/sidebar/branches.cljs:987 +msgid "workspace.branches.list.error" +msgstr "Couldn't load branches" + #: src/app/main/ui/workspace/sidebar/branches.cljs:945 msgid "workspace.branches.list.tab.active" msgstr "Active" @@ -7132,7 +7136,7 @@ msgstr "Mine" msgid "workspace.branches.list.title" msgstr "Branches - %s" -#, unused +#: src/app/main/ui/workspace/branch_switcher.cljs:199, src/app/main/ui/workspace/sidebar/branches.cljs:982 msgid "workspace.branches.loading" msgstr "Loading branches…"