github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 14:00:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

✨ Load the branch list only when the switcher opens (#12571)

Browse Source 
* ✨ 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 <alvorithm@teje.ro>

* ✨ 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 <alvorithm@teje.ro>

* ✨ 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 <alvorithm@teje.ro>

* 📚 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 <alvorithm@teje.ro>

* 🐛 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 <alvorithm@teje.ro>

---------

Signed-off-by: Álvaro Tejero Cantero <alvorithm@teje.ro>
This commit is contained in:
Álvaro Tejero-Cantero 2026-10-22 20:50:58 +02:00 committed by GitHub
parent d0414481ef
commit d669eec0c0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 579 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
backend/resources/climit.edn
View File

@ -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.

36
backend/src/app/rpc/commands/files_branch.clj
View File

@ -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

49
backend/test/backend_tests/rpc_file_branch_test.clj
View File

@ -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)

3
frontend/playwright/data/branches/get-file-branch-count-two.json Normal file
View File

@ -0,0 +1,3 @@
{
"~:count": 2
}

32
frontend/playwright/data/branches/get-file-branches-two.json Normal file
View File

@ -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
}
]

151
frontend/playwright/ui/specs/branch-switcher.spec.js Normal file
View File

@ -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();
});

4
frontend/playwright/ui/specs/branches-library.spec.js
View File

@ -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();

59
frontend/src/app/main/data/workspace/branches.cljs
View File

@ -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

9
frontend/src/app/main/data/workspace/pull_requests.cljs
View File

@ -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))]

109
frontend/src/app/main/ui/workspace/branch_switcher.cljs
View File

@ -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

10
frontend/src/app/main/ui/workspace/branch_switcher.scss
View File

@ -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");

30
frontend/src/app/main/ui/workspace/sidebar/branches.cljs
View File

@ -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))]

122
frontend/test/frontend_tests/data/workspace_branches_test.cljs
View File

@ -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)]

8
frontend/translations/en.po
View File

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