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…"