From c1dec5d4f0233afedef1f01fed4e0248f5638db1 Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Fri, 23 Oct 2026 10:32:21 +0200 Subject: [PATCH] :sparkles: Add merge reviews and tidy up the branching workspace --- .../app/email/review-request/en.html | 2 +- .../resources/app/email/review-request/en.txt | 2 +- backend/src/app/migrations.clj | 5 +- .../sql/0806-add-file-branch-merge-table.sql | 33 ++ backend/src/app/rpc/commands/files_branch.clj | 120 ++++- .../rpc_file_branch_oplog_test.clj | 81 +-- .../backend_tests/rpc_file_branch_test.clj | 120 +++-- .../rpc_file_pull_request_test.clj | 84 +-- .../images/icons/git-branch-minus.svg | 6 + .../images/icons/git-compare-arrows.svg | 8 + frontend/resources/images/icons/git-graph.svg | 8 + .../images/icons/git-merge-conflict.svg | 7 + .../icons/git-pull-request-create-arrow.svg | 8 + .../src/app/main/data/workspace/branches.cljs | 97 ++-- .../main/ui/ds/foundations/assets/icon.cljs | 5 + .../app/main/ui/ds/product/branch_row.cljs | 42 +- .../src/app/main/ui/ds/product/branch_row.mdx | 4 + .../main/ui/workspace/branch_switcher.cljs | 56 +- .../main/ui/workspace/branch_switcher.scss | 15 +- .../src/app/main/ui/workspace/main_menu.cljs | 4 +- .../main/ui/workspace/sidebar/branches.cljs | 486 ++++++++++++------ .../main/ui/workspace/sidebar/branches.scss | 10 +- .../ui/workspace/sidebar/pull_requests.cljs | 6 +- .../main/ui/workspace/sidebar/reviews.cljs | 405 +++++++++++++++ .../main/ui/workspace/sidebar/reviews.scss | 240 +++++++++ .../data/workspace_branches_test.cljs | 71 ++- frontend/translations/en.po | 131 +++-- 27 files changed, 1555 insertions(+), 501 deletions(-) create mode 100644 backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql create mode 100644 frontend/resources/images/icons/git-branch-minus.svg create mode 100644 frontend/resources/images/icons/git-compare-arrows.svg create mode 100644 frontend/resources/images/icons/git-graph.svg create mode 100644 frontend/resources/images/icons/git-merge-conflict.svg create mode 100644 frontend/resources/images/icons/git-pull-request-create-arrow.svg create mode 100644 frontend/src/app/main/ui/workspace/sidebar/reviews.cljs create mode 100644 frontend/src/app/main/ui/workspace/sidebar/reviews.scss diff --git a/backend/resources/app/email/review-request/en.html b/backend/resources/app/email/review-request/en.html index a9f1651d5a..1301724caa 100644 --- a/backend/resources/app/email/review-request/en.html +++ b/backend/resources/app/email/review-request/en.html @@ -186,7 +186,7 @@
- {{ source-user }} has requested your review on the pull request "{{ pull-request-title }}" ({{ pull-request-route }}).
+ {{ source-user }} has requested your review on the design review "{{ pull-request-title }}" ({{ pull-request-route }}). diff --git a/backend/resources/app/email/review-request/en.txt b/backend/resources/app/email/review-request/en.txt index 84e12bb126..fb29439fc3 100644 --- a/backend/resources/app/email/review-request/en.txt +++ b/backend/resources/app/email/review-request/en.txt @@ -1,6 +1,6 @@ Hello {{name|abbreviate:25}}! -{{ source-user }} has requested your review on the pull request "{{ pull-request-title }}" ({{ pull-request-route }}). +{{ source-user }} has requested your review on the design review "{{ pull-request-title }}" ({{ pull-request-route }}). -- diff --git a/backend/src/app/migrations.clj b/backend/src/app/migrations.clj index 4ffc31dadf..ddb2502c23 100644 --- a/backend/src/app/migrations.clj +++ b/backend/src/app/migrations.clj @@ -542,7 +542,10 @@ :fn (mg/resource "app/migrations/sql/0804-add-file-branch-change-changes-count.sql")} {:name "0805-stamp-file-branch-change-changes-count" - :fn mg0805/migrate}]) + :fn mg0805/migrate} + + {:name "0806-add-file-branch-merge-table" + :fn (mg/resource "app/migrations/sql/0806-add-file-branch-merge-table.sql")}]) (defn apply-migrations! [pool name migrations] diff --git a/backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql b/backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql new file mode 100644 index 0000000000..63879fcdf4 --- /dev/null +++ b/backend/src/app/migrations/sql/0806-add-file-branch-merge-table.sql @@ -0,0 +1,33 @@ +-- The record of every merge of a branch into its source file (main): who +-- merged it, when, the comment they left and, when the merge accepted a +-- design review, that review. A merge deletes the branch, and the objects +-- GC later deletes its file, which takes the `file_branch` and +-- `file_pull_request` rows with it through their cascades. So this table +-- keeps its own copy of the names it shows and holds no foreign key to +-- either table: the record outlives both. It goes away with main. + +CREATE TABLE file_branch_merge ( + id uuid PRIMARY KEY, + + created_at timestamptz NOT NULL DEFAULT clock_timestamp(), + + -- main: the file the branch was merged into + file_id uuid NOT NULL REFERENCES file(id) ON DELETE CASCADE DEFERRABLE, + + -- the merged branch, by id and by the name it had when it was merged + file_branch_id uuid NOT NULL, + branch_name text NOT NULL, + + merged_by uuid NULL REFERENCES profile(id) ON DELETE SET NULL DEFERRABLE, + comment text NULL, + + -- the design review the merge accepted, if any + pull_request_id uuid NULL, + pull_request_title text NULL +); + +CREATE INDEX file_branch_merge__file_id__created_at__idx + ON file_branch_merge(file_id, created_at DESC); + +CREATE INDEX file_branch_merge__merged_by__idx + ON file_branch_merge(merged_by); diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index 5adcfa7e4b..8dfd51f4c2 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -403,6 +403,36 @@ :profile-id profile-id :session-id session-id}})) +;; --- Helpers: merge record + +(def ^:private sql:get-open-branch-pull-request + "SELECT id, title + FROM file_pull_request + WHERE file_branch_id = ? + AND status = 'open' + AND deleted_at IS NULL") + +(defn- record-merge! + "Insert the record of a merge of `branch` into its source file (see the + `file_branch_merge` migration). The open pull request of the branch is + the design review the merge accepts, so it is read here, before the + merge closes it. A blank comment is stored as no comment. Must run + inside the merge transaction." + [conn {:keys [id source-file-id name]} {:keys [profile-id comment created-at]}] + (let [pr (db/exec-one! conn [sql:get-open-branch-pull-request id]) + comment (when-not (str/blank? comment) (str/trim comment))] + (db/insert! conn :file-branch-merge + {:id (uuid/next) + :created-at created-at + :file-id source-file-id + :file-branch-id id + :branch-name name + :merged-by profile-id + :comment comment + :pull-request-id (:id pr) + :pull-request-title (:title pr)} + {::db/return-keys false}))) + ;; --- COMMAND: create-file-branch (defn- check-branch-name! @@ -593,6 +623,7 @@ JOIN file AS sf ON (sf.id = fb.source_file_id) WHERE fb.source_file_id = ? AND fb.deleted_at IS NULL + AND fb.status != 'merged' AND (?::boolean OR fb.status = 'open') ORDER BY fb.created_at DESC") @@ -958,11 +989,9 @@ ;; deselected, which the merge resolves to the target side [:excluded {:optional true} [::sm/set :string]] [:expected-main-revn {:optional true} ::sm/int] - ;; when false (the default) the branch and its file are logically - ;; deleted right after a successful merge, in the SAME transaction (so - ;; merged copies do not pile up); when true the branch is kept as - ;; "merged", staying readable with status merged until it is deleted. - [:keep-branch {:optional true} ::sm/boolean]]) + ;; what the user says about the merge; kept in its record + ;; (`file_branch_merge`), shown in the list of merges of main + [:comment {:optional true} [:string {:max 4000}]]]) (sv/defmethod ::merge-file-branch "Merge a branch into its source file (main). @@ -974,15 +1003,16 @@ kinds the engine cannot translate (`bm/unsupported-kinds`) make it return `{:status :unsupported}` so no change is silently dropped. Otherwise it takes a safety snapshot of main, applies the merge to main through the - production change pipeline, marks the branch merged and notifies open - clients via msgbus." + production change pipeline, records the merge with its `comment` + (`file_branch_merge`), deletes the branch and its file and notifies + open clients via msgbus." {::doc/added "2.16" ::webhooks/event? true ::sm/params schema:merge-file-branch ::climit/id [[:merge-file-branch/by-profile ::rpc/profile-id] [:merge-file-branch/global]]} [{:keys [::mbus/msgbus] :as cfg} - {:keys [::rpc/profile-id ::rpc/session-id branch-id resolutions expected-main-revn keep-branch excluded]}] + {:keys [::rpc/profile-id ::rpc/session-id branch-id resolutions expected-main-revn comment excluded]}] (bp/with-policies (check-branching-enabled!) (let [branch (db/get* cfg :file-branch {:id branch-id})] @@ -1011,7 +1041,7 @@ ;; advisory lock the normal update-file path takes; stable order ;; to avoid deadlocks): without the branch lock, edits saved to ;; the branch while the merge runs would be silently lost when - ;; the branch is marked merged (and possibly deleted) below. + ;; the branch is marked merged and deleted below. (run! (partial db/xact-lock! conn) (sort [main-id branch-file-id])) ;; the row read before the lock is stale by now: a competing @@ -1073,15 +1103,16 @@ finish-branch! (fn [ts] - ;; mark merged. The pinned base snapshot stays for a - ;; KEPT branch (its state is derived from that - ;; snapshot and the op log, so releasing the pin - ;; would leave it unreadable once the snapshot GC - ;; runs); delete-branch! releases it on the delete - ;; path. Unless the user chose to keep it the branch - ;; and its file are deleted in this same transaction + ;; record the merge, mark the branch merged and + ;; delete it and its file in this same transaction ;; (no client-driven second call, no window where a - ;; crash leaves a stale copy). + ;; crash leaves a stale copy); delete-branch! + ;; releases the pinned base snapshot. The record is + ;; written first: it reads the open pull request the + ;; lines below close. + (record-merge! conn branch {:profile-id profile-id + :comment comment + :created-at ts}) (db/update! conn :file-branch {:status "merged" :merged-at ts @@ -1097,10 +1128,9 @@ {:profile-id profile-id :status "merged" :deleted-at (ct/in-future delay)}) - (when-not keep-branch - (vswap! messages conj - (delete-branch! cfg branch {:profile-id profile-id - :session-id session-id})))))] + (vswap! messages conj + (delete-branch! cfg branch {:profile-id profile-id + :session-id session-id}))))] (cond (seq unresolved) (audited {:status :conflicts :conflicts conflicts :main-revn (:revn main-file)} @@ -1784,6 +1814,7 @@ "SELECT count(*) AS count FROM file_branch AS fb WHERE fb.deleted_at IS NULL + AND fb.status != 'merged' AND fb.source_file_id = coalesce((SELECT b.source_file_id FROM file_branch AS b WHERE b.branch_file_id = ? @@ -1791,8 +1822,8 @@ ?)") (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 + "How many branches the file has, archived 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 @@ -1810,6 +1841,49 @@ (select-keys [:count])))) {:count 0})) +;; --- COMMAND QUERY: get-file-branch-merges + +(def ^:private schema:get-file-branch-merges + [:map {:title "get-file-branch-merges"} + [:file-id ::sm/uuid]]) + +(def ^:private sql:resolve-source-file + "SELECT coalesce((SELECT b.source_file_id + FROM file_branch AS b + WHERE b.branch_file_id = ? + AND b.deleted_at IS NULL), + ?) AS id") + +(def ^:private sql:get-file-branch-merges + "SELECT fbm.id, + fbm.created_at, + fbm.file_id, + fbm.file_branch_id, + fbm.branch_name, + fbm.merged_by, + fbm.comment, + fbm.pull_request_id, + fbm.pull_request_title + FROM file_branch_merge AS fbm + WHERE fbm.file_id = ? + ORDER BY fbm.created_at DESC") + +(sv/defmethod ::get-file-branch-merges + "List the merges into a file, newest first: each one names the merged + branch, who merged it and when, the comment left with it and the design + review it accepted, if any. When the file is itself a branch, the + merges into its source (main) are listed. The records outlive the + branches they name (see the `file_branch_merge` migration)." + {::doc/added "2.16" + ::sm/params schema:get-file-branch-merges} + [cfg {:keys [::rpc/profile-id file-id]}] + (check-branching-enabled!) + (db/run! cfg + (fn [{:keys [::db/conn] :as cfg}] + (let [main-id (:id (db/exec-one! conn [sql:resolve-source-file file-id file-id]))] + (files/check-read-permissions! cfg profile-id main-id) + (db/exec! conn [sql:get-file-branch-merges main-id]))))) + ;; --- COMMAND: update-file-branch (rename / description) (def ^:private schema:update-file-branch diff --git a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj index ae4b6d3a3a..b469c63862 100644 --- a/backend/test/backend_tests/rpc_file_branch_oplog_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_oplog_test.clj @@ -15,7 +15,6 @@ [app.common.data :as d] [app.common.features :as cfeat] [app.common.files.migrations :as fmg] - [app.common.time :as ct] [app.common.types.shape :as cts] [app.common.uuid :as uuid] [app.config :as cf] @@ -172,70 +171,6 @@ mf (th/db-get :file {:id (:id file)})] (t/is (= (:revn mf) (:base-revn row)))))))) -(t/deftest merge-with-keep-branch-leaves-the-branch-readable - (with-redefs [cf/flags (conj cf/flags :branching)] - (let [profile (th/create-profile* 1 {: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 (create-branch* profile (:id file) "oplog-keep") - branch-file-id (:branch-file-id create) - branch-id (:id create) - color-id (uuid/random)] - - (apply-change* profile branch-file-id - {:type :add-color - :color {:id color-id :name "C" :color "#112233" :opacity 1}}) - - (let [out (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] - (t/is (nil? (:error out))) - (t/is (= :merged (-> out :result :status)))) - - (t/testing "the kept branch still derives its state" - (let [out (th/command! {::th/type :get-file - ::rpc/profile-id (:id profile) - :id branch-file-id})] - (t/is (nil? (:error out))) - (t/is (contains? (-> out :result :data :colors) color-id)))) - - (t/testing "the base pin survives while the branch is kept" - (let [rows (->> (th/db-query :file-change {:file-id (:id file)}) - (filter #(some-> (:label %) (str/starts-with? "branch-base/"))))] - (t/is (seq rows)) - (t/is (every? (fn [{:keys [deleted-at]}] - (or (nil? deleted-at) - (ct/is-after? deleted-at (ct/in-future {:days 400})))) - rows))))))) - -(t/deftest a-kept-merged-branch-refuses-saves - (with-redefs [cf/flags (conj cf/flags :branching)] - (let [profile (th/create-profile* 1 {: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 (create-branch* profile (:id file) "oplog-merged") - branch-file-id (:branch-file-id create) - branch-id (:id create)] - - (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true}) - - (let [out (apply-change* profile branch-file-id - {:type :add-color - :color {:id (uuid/random) :name "C" :color "#112233" :opacity 1}})] - (t/is (th/ex-of-type? (:error out) :validation)) - (t/is (th/ex-of-code? (:error out) :branch-merged))) - - (t/testing "nothing reaches the op log" - (t/is (empty? (oplog-rows branch-file-id))))))) - (defn- add-color* [profile file-id] (apply-change* profile file-id {:type :add-color @@ -1408,8 +1343,7 @@ ;; a media row MAIN owns (let [merge (-> (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true}) + :branch-id branch-id}) :result)] (t/is (= :merged (:status merge))) (let [ref (-> (derived-obj profile main-id page-id shape-id) @@ -1462,14 +1396,13 @@ ;; drops the row (it filters by branch ownership) and main ends up ;; referencing media it does not own. (with-redefs [cf/flags (conj cf/flags :branching)] - (let [{:keys [profile main-id branch-id branch-file-id page-id shape-id]} + (let [{:keys [profile main-id branch-id page-id shape-id]} (foreign-media-setup) out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status))) @@ -1477,10 +1410,4 @@ (let [ref (-> (derived-obj profile main-id page-id shape-id) media-ref) row (th/db-get :file-media-object {:id ref})] (t/is (some? row)) - (t/is (= main-id (:file-id row)))) - - ;; the branch keeps deriving its own state from the kept op log - (let [ref (-> (derived-obj profile branch-file-id page-id shape-id) media-ref) - row (th/db-get :file-media-object {:id ref})] - (t/is (some? row)) - (t/is (= branch-file-id (:file-id row))))))) + (t/is (= main-id (:file-id row))))))) diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index bc01053a30..fed5b53d70 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -797,31 +797,69 @@ (ct/is-before? deleted-at (ct/in-future {:days 400})))) rows)))))) -(t/deftest merge-with-keep-branch-archives-it +(t/deftest merge-records-itself-and-deletes-the-branch (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) proj-id (:default-project-id profile) file (th/create-file* 1 {:profile-id (:id profile) :project-id proj-id}) - create (:result (th/command! {::th/type :create-file-branch + create! (fn [name] + (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name name}))) + merge! (fn [branch-id params] + (th/command! (merge {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :file-id (:id file) - :name "kept"})) - branch-id (:id create) - branch-file-id (:branch-file-id create)] + :branch-id branch-id} + params))) + first-b (create! "first") + second-b (create! "second")] - (let [out (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + (let [out (merge! (:id first-b) {:comment " Ships the new header "})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status)))) - (let [[row] (th/db-query :file-branch {:id branch-id}) - bf (th/db-get :file {:id branch-file-id})] - (t/is (= "merged" (:status row))) - (t/is (nil? (:deleted-at row))) - (t/is (nil? (:deleted-at bf))))))) + (t/testing "the merged branch and its file are deleted" + (let [[row] (th/db-query :file-branch {:id (:id first-b)}) + bf (th/db-get :file {:id (:branch-file-id first-b)} {::db/remove-deleted false})] + (t/is (= "merged" (:status row))) + (t/is (some? (:deleted-at row))) + (t/is (some? (:deleted-at bf))))) + + (let [out (merge! (:id second-b) {:comment " "})] + (t/is (nil? (:error out)))) + + (t/testing "the merges are listed newest first, from main or a branch" + (let [out (th/command! {::th/type :get-file-branch-merges + ::rpc/profile-id (:id profile) + :file-id (:id file)}) + merges (:result out)] + (t/is (nil? (:error out))) + (t/is (= ["second" "first"] (mapv :branch-name merges))) + (t/is (every? #(= (:id profile) (:merged-by %)) merges)) + (t/is (every? #(= (:id file) (:file-id %)) merges)) + (t/is (= [nil "Ships the new header"] (mapv :comment merges)) + "the comment is trimmed, and a blank one is no comment") + (t/is (every? (comp nil? :pull-request-id) merges)))) + + (t/testing "a merged branch leaves the branch listing" + (let [out (th/command! {::th/type :get-file-branches + ::rpc/profile-id (:id profile) + :file-id (:id file) + :include-archived true})] + (t/is (empty? (:result out)))))))) + +(t/deftest the-merges-of-a-file-need-read-permissions + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [profile (th/create-profile* 1 {:is-active true}) + other (th/create-profile* 2 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + out (th/command! {::th/type :get-file-branch-merges + ::rpc/profile-id (:id other) + :file-id (:id file)})] + (t/is (th/ex-of-type? (:error out) :not-found))))) (t/deftest merge-requires-main-edition-permissions (with-redefs [cf/flags (conj cf/flags :branching)] @@ -1185,8 +1223,7 @@ (t/testing "merging brings the new media with a MAIN-owned row" (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status)))) @@ -1243,8 +1280,7 @@ (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (nil? (:error out))) (t/is (= :merged (-> out :result :status)))) @@ -1860,9 +1896,9 @@ (t/is (= "merged" (:status row))) (t/is (some? (:deleted-at row)))))))))) -(t/deftest delete-keeps-the-status-the-branch-carries - ;; deleting a kept merged branch must not rewrite its status: the - ;; record is the only place left that says the branch was merged +(t/deftest a-merged-branch-is-gone-for-delete + ;; a merge deletes its branch: deleting it again finds nothing and does + ;; not rewrite the status that says the branch was merged (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) proj-id (:default-project-id profile) @@ -1871,7 +1907,7 @@ create (:result (th/command! {::th/type :create-file-branch ::rpc/profile-id (:id profile) :file-id (:id file) - :name "kept"})) + :name "merged"})) branch-id (:id create) branch-file-id (:branch-file-id create) cid (uuid/random)] @@ -1879,14 +1915,12 @@ {:type :add-color :color {:id cid :name "C" :color "#112233" :opacity 1}}) (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (= :merged (-> out :result :status)))) (let [out (th/command! {::th/type :delete-file-branch ::rpc/profile-id (:id profile) :id branch-id})] - (t/is (nil? (:error out))) - (t/is (= :deleted (-> out :result :status)))) + (t/is (= :branch-not-found (-> out :error ex-data :code)))) (let [[row] (th/db-query :file-branch {:id branch-id})] (t/is (= "merged" (:status row))) (t/is (some? (:deleted-at row))))))) @@ -2298,8 +2332,8 @@ (t/deftest merge-refuses-a-branch-merged-under-the-lock ;; two merges of one open branch: the winner commits before the ;; outer merge takes any lock. The outer merge must refuse instead of - ;; trusting its stale "open" row and deleting the branch the winner - ;; kept. + ;; trusting its stale "open" row: merging the branch a second time, + ;; recording a second merge and deleting it again. (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) proj-id (:default-project-id profile) @@ -2321,26 +2355,26 @@ db/xact-lock! (fn [conn id] ;; the winning merge runs to completion BEFORE - ;; the outer one takes any lock, and keeps the - ;; branch so the outer merge still has its file + ;; the outer one takes any lock (when (compare-and-set! raced? false true) (reset! merged (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true}))) + :branch-id branch-id}))) (orig-lock conn id))] (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) :branch-id branch-id})] - (t/is (= :branch-not-open (-> out :error ex-data :code))))) + (t/is (= :branch-not-found (-> out :error ex-data :code))))) (t/is (= :merged (-> @merged :result :status))) (t/testing "main holds the first merge's revn only" (t/is (= (-> @merged :result :revn) (:revn (first (th/db-query :file {:id (:id file)})))))) - (t/testing "the branch the winning merge kept is still kept" + (t/testing "the winning merge is the only one recorded" + (t/is (= 1 (count (th/db-query :file-branch-merge {:file-branch-id branch-id}))))) + (t/testing "the branch is merged and deleted once" (let [[row] (th/db-query :file-branch {:id branch-id})] (t/is (= "merged" (:status row))) - (t/is (nil? (:deleted-at row))))))))) + (t/is (some? (:deleted-at row))))))))) (t/deftest merge-releases-the-base-the-lock-shows ;; the merge releases the base snapshot pin (on the delete path), and @@ -2514,15 +2548,17 @@ :expected-main-revn 9999})] (t/is (= :file-modified (-> out :error ex-data :code))) (t/is (empty? @seen))) - ;; the kept branch means :file-merged is the only message + ;; the merge deletes the branch: its file's clients hear of the + ;; deletion and main's of the merge (let [out (th/command! {::th/type :merge-file-branch ::rpc/profile-id (:id profile) - :branch-id branch-id - :keep-branch true})] + :branch-id branch-id})] (t/is (= :merged (-> out :result :status)))) - (let [[{:keys [message main-revn]}] @seen] - (t/is (= 1 (count @seen))) - (t/is (= :file-merged (:type message))) + (let [by-type (into {} (map (juxt (comp :type :message) identity)) @seen) + {:keys [message main-revn]} (get by-type :file-merged)] + (t/is (= 2 (count @seen))) + (t/is (= #{:file-deleted :file-merged} (set (keys by-type)))) + (t/is (= branch-file-id (get-in by-type [:file-deleted :message :file-id]))) ;; the merge was already committed when the message left (t/is (= (:revn message) main-revn))))))) diff --git a/backend/test/backend_tests/rpc_file_pull_request_test.clj b/backend/test/backend_tests/rpc_file_pull_request_test.clj index fc082eee7d..496695650f 100644 --- a/backend/test/backend_tests/rpc_file_pull_request_test.clj +++ b/backend/test/backend_tests/rpc_file_pull_request_test.clj @@ -492,7 +492,14 @@ (t/is (= "merged" (:status row))) (t/is (some? (:closed-at row))) ;; the row survives as history even though the branch is gone - (t/is (nil? (:deleted-at row)))))) + (t/is (nil? (:deleted-at row)))) + ;; the record of the merge names the review it accepted, so the + ;; list of merges keeps it once the review is gone + (let [merges (:result (th/command! {::th/type :get-file-branch-merges + ::rpc/profile-id (:id author) + :file-id (:id file)}))] + (t/is (= [(:id pr)] (mapv :pull-request-id merges))) + (t/is (= [(:title pr)] (mapv :pull-request-title merges)))))) (t/testing "deleting the branch closes the pull request" (let [branch (create-branch* author file "delete-me") @@ -585,81 +592,6 @@ (t/is (= :branch-not-open (-> out :error ex-data :code))) (t/is (= "closed" (:status (first (th/db-query :file-pull-request {:id (:id pr)})))))))))))) -(t/deftest create-serializes-against-a-kept-concurrent-merge - ;; same race, but the merge keeps the branch: its row stays readable - ;; with status "merged" and its file is not deleted, so without the - ;; re-read the create sails past the stale status check and INSERTS a - ;; pull request for a branch the merge already closed. - (with-redefs [cf/flags pr-flags] - (let [author (th/create-profile* 1 {:is-active true}) - file (th/create-file* 1 {:profile-id (:id author) - :project-id (:default-project-id author)}) - branch (create-branch* author file "contested") - branch-id (:id branch) - branch-file-id (:branch-file-id branch)] - (add-color* author branch-file-id "Brand") - (let [orig-lock db/xact-lock! - merging? (atom false) - merged (atom nil)] - (with-redefs [db/xact-lock! - (fn [conn id] - ;; the first lock the create takes is the signal: - ;; merge the same branch through the pool in its - ;; own transaction BEFORE the create locks - (when (compare-and-set! merging? false true) - (reset! merged (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id author) - :branch-id branch-id - :keep-branch true}))) - (orig-lock conn id))] - (let [out (th/command! {::th/type :create-pull-request - ::rpc/profile-id (:id author) - :branch-id branch-id - :title "review me"})] - (t/is (= :merged (-> @merged :result :status))) - (t/is (= "merged" (:status (first (th/db-query :file-branch {:id branch-id}))))) - (t/is (= :branch-not-open (-> out :error ex-data :code))) - (t/is (= [] (th/db-query :file-pull-request {:file-branch-id branch-id}))))))))) - -(t/deftest reopen-serializes-against-a-kept-concurrent-merge - ;; same race for the reopen: the kept merge leaves the branch row - ;; readable with status "merged", so without the re-read the reopen - ;; sails past the stale status check and REOPENS the closed pull - ;; request over a branch the merge already closed. - (with-redefs [cf/flags pr-flags] - (let [author (th/create-profile* 1 {:is-active true}) - file (th/create-file* 1 {:profile-id (:id author) - :project-id (:default-project-id author)}) - branch (create-branch* author file "contested") - branch-id (:id branch) - branch-file-id (:branch-file-id branch)] - (add-color* author branch-file-id "Brand") - (let [pr (:result (create-pr* author branch-id {}))] - (t/is (nil? (:error (th/command! {::th/type :close-pull-request - ::rpc/profile-id (:id author) - :id (:id pr)})))) - (let [orig-lock db/xact-lock! - merging? (atom false) - merged (atom nil)] - (with-redefs [db/xact-lock! - (fn [conn id] - ;; the first lock the reopen takes is the signal: - ;; merge the same branch through the pool in its - ;; own transaction BEFORE the reopen locks - (when (compare-and-set! merging? false true) - (reset! merged (th/command! {::th/type :merge-file-branch - ::rpc/profile-id (:id author) - :branch-id branch-id - :keep-branch true}))) - (orig-lock conn id))] - (let [out (th/command! {::th/type :reopen-pull-request - ::rpc/profile-id (:id author) - :id (:id pr)})] - (t/is (= :merged (-> @merged :result :status))) - (t/is (= "merged" (:status (first (th/db-query :file-branch {:id branch-id}))))) - (t/is (= :branch-not-open (-> out :error ex-data :code))) - (t/is (= "closed" (:status (first (th/db-query :file-pull-request {:id (:id pr)})))))))))))) - (t/deftest closed-pull-requests-listing (with-redefs [cf/flags pr-flags] (let [author (th/create-profile* 1 {:is-active true}) diff --git a/frontend/resources/images/icons/git-branch-minus.svg b/frontend/resources/images/icons/git-branch-minus.svg new file mode 100644 index 0000000000..e1d7392190 --- /dev/null +++ b/frontend/resources/images/icons/git-branch-minus.svg @@ -0,0 +1,6 @@ + + + + + + diff --git a/frontend/resources/images/icons/git-compare-arrows.svg b/frontend/resources/images/icons/git-compare-arrows.svg new file mode 100644 index 0000000000..4bb584c4ec --- /dev/null +++ b/frontend/resources/images/icons/git-compare-arrows.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/frontend/resources/images/icons/git-graph.svg b/frontend/resources/images/icons/git-graph.svg new file mode 100644 index 0000000000..715b79c033 --- /dev/null +++ b/frontend/resources/images/icons/git-graph.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/frontend/resources/images/icons/git-merge-conflict.svg b/frontend/resources/images/icons/git-merge-conflict.svg new file mode 100644 index 0000000000..a0ecbf4632 --- /dev/null +++ b/frontend/resources/images/icons/git-merge-conflict.svg @@ -0,0 +1,7 @@ + + + + + + + diff --git a/frontend/resources/images/icons/git-pull-request-create-arrow.svg b/frontend/resources/images/icons/git-pull-request-create-arrow.svg new file mode 100644 index 0000000000..472b28b162 --- /dev/null +++ b/frontend/resources/images/icons/git-pull-request-create-arrow.svg @@ -0,0 +1,8 @@ + + + + + + + + diff --git a/frontend/src/app/main/data/workspace/branches.cljs b/frontend/src/app/main/data/workspace/branches.cljs index be5396dc6f..ee5c27ad60 100644 --- a/frontend/src/app/main/data/workspace/branches.cljs +++ b/frontend/src/app/main/data/workspace/branches.cljs @@ -18,6 +18,7 @@ [app.util.dom :as dom] [app.util.i18n :refer [tr]] [beicon.v2.core :as rx] + [cuerdas.core :as str] [potok.v2.core :as ptk])) (defn- reload-file-window @@ -137,6 +138,33 @@ (rx/catch (fn [_] (rx/of (update-branches-state {:status :error}))))))))) +(defn- set-branch-merges + [merges-state] + (ptk/reify ::set-branch-merges + ptk/UpdateEvent + (update [_ state] + (assoc state :workspace-branch-merges merges-state)))) + +(defn fetch-branch-merges + "Load the merges into the open file's main, newest first, into + `:workspace-branch-merges` (`{:status :loading|:loaded|:error}` plus + `:data`): who merged each branch, when, and the comment left with it. + The command resolves a branch to its main, so it is asked about the + open file as is. A failure is kept as an error, never as an empty list, + so it cannot read as a file with no merges." + [] + (ptk/reify ::fetch-branch-merges + ptk/UpdateEvent + (update [_ state] + (assoc state :workspace-branch-merges {:status :loading})) + + ptk/WatchEvent + (watch [_ state _] + (when-let [file-id (:current-file-id state)] + (->> (rp/cmd! :get-file-branch-merges {:file-id file-id}) + (rx/map #(set-branch-merges {:status :loaded :data %})) + (rx/catch (fn [_] (rx/of (set-branch-merges {:status :error}))))))))) + (defn fetch-branching-limits "Load the size limits this instance refuses beyond, so the panel can state them before a user meets one. The numbers live in the backend @@ -254,15 +282,24 @@ (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error")))))))))) (defn delete-branch - [id] - (assert (uuid? id) "expected valid uuid for `id`") - (ptk/reify ::delete-branch - ptk/WatchEvent - (watch [_ _ _] - (->> (rp/cmd! :delete-file-branch {:id id}) - (rx/mapcat (fn [_] (rx/of (ntf/success (tr "workspace.branches.lifecycle.deleted")) - (fetch-branches)))) - (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error"))))))))) + "Delete a branch. Deleting the open branch takes its file away, so the + caller passes `open-file-id` (the file it comes from) to land there once + the branch is gone; this session is not sent away by the deletion it + made itself." + ([id] (delete-branch id nil)) + ([id open-file-id] + (assert (uuid? id) "expected valid uuid for `id`") + (ptk/reify ::delete-branch + ptk/WatchEvent + (watch [_ _ _] + (->> (rp/cmd! :delete-file-branch {:id id}) + (rx/mapcat (fn [_] (rx/concat + (rx/of (ntf/success (tr "workspace.branches.lifecycle.deleted")) + (fetch-branches)) + (if (uuid? open-file-id) + (rx/of (open-branch open-file-id)) + (rx/empty))))) + (rx/catch (fn [cause] (rx/of (refusal-notification cause (tr "workspace.branches.lifecycle.error")))))))))) (defn materialize-branch "Turn a branch into an ordinary file: `branch-file-id` is the branch's @@ -361,18 +398,19 @@ (defn show-branches "The entry point of the File menu and the Ctrl/Cmd+Alt+B shortcut. A - file that has branches gets the branch switcher; one with none gets the - create-branch dialog, since the switcher only exists once a branch does. - No-op when the branching feature flag is disabled." + file that has branches gets the branches dialog (the full list, the + same the switcher's \"All branches\" opens, which shows this shortcut); + one with none gets the create-branch dialog. No-op when the branching + feature flag is disabled." [] (ptk/reify ::show-branches ptk/WatchEvent (watch [_ state _] (when (contains? cf/flags :branching) (if (has-branches? state) - (rx/of (toggle-branch-switcher true)) - (let [file (get-in state [:files (:current-file-id state)])] - (rx/of (modal/show :create-branch {:file-name (:name file)})))))))) + (rx/of (toggle-branch-switcher false) + (modal/show :branches-list {})) + (rx/of (modal/show :create-branch {}))))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; COMPARE (read-only 3-way diff) @@ -524,7 +562,7 @@ `main-revn` is main's revn when the command computed those conflicts. It is stored with them because the modal's own diff fetch can still be in flight when the user applies; see `conflict-main-revn`." - [{:keys [branch mode conflicts main-revn excluded]}] + [{:keys [branch mode conflicts main-revn excluded comment]}] (ptk/reify ::open-conflict-resolutions ptk/UpdateEvent (update [_ state] @@ -536,11 +574,13 @@ ptk/WatchEvent (watch [_ _ _] ;; `excluded` is the per-change selection the merge/update attempt - ;; was made with; the resolution modal applies it on retry + ;; was made with, and `comment` what the user said about the merge; + ;; the resolution modal sends both again on retry (rx/of (modal/show :branch-conflicts {:branch branch :mode mode :conflicts conflicts - :excluded excluded}))))) + :excluded excluded + :comment comment}))))) (defn- conflict-main-revn "Main's revn when the server computed the conflict set stored for @@ -662,16 +702,14 @@ (defn merge-branch "Merge `branch` (a branch row/context map with at least `:id`) into main. - On success the branch is marked merged server-side and open clients - reload main via the `:file-merged` msgbus event. + On success the server records the merge and deletes the branch and its + file, and open clients reload main via the `:file-merged` msgbus event. Options: - `:resolutions` — optional `{entity-id (:main|:branch)}` map used to resolve conflicts before integrating. - - `:keep-branch` — when true the merged branch is kept (it shows up - under \"Archived\"); when false (default) the server deletes the - branch and its file in the same transaction, so merged copies do not - pile up and eat disk space. + - `:comment` — what the user says about the merge, kept with its + record and shown in the reviews dialog. - `:excluded` — the per-change selection (change keys, `app.common.files.branch-merge/change-key`) naming the changes the user deselected, which main keeps its own value for. @@ -683,7 +721,7 @@ before that diff has loaded carry the revn of the conflict answer instead (`conflict-main-revn`). Unresolved conflicts open the resolution modal." ([branch] (merge-branch branch nil)) - ([branch {:keys [resolutions keep-branch excluded]}] + ([branch {:keys [resolutions comment excluded]}] (assert (uuid? (:id branch)) "expected a branch row with a valid `:id`") (ptk/reify ::merge-branch ptk/WatchEvent @@ -700,7 +738,7 @@ (cond-> {:branch-id branch-id} (seq resolutions) (assoc :resolutions resolutions) (seq excluded) (assoc :excluded excluded) - keep-branch (assoc :keep-branch true) + (not (str/blank? comment)) (assoc :comment (str/trim comment)) (some? main-revn) (assoc :expected-main-revn main-revn))) (rx/mapcat (fn [{:keys [status source-file-id conflicts] :as answer}] @@ -708,15 +746,16 @@ ;; the merged result lives in main: take the user there ;; to see it (navigate if elsewhere, hard-reload if ;; already on main). Other clients reload via the - ;; `:file-merged` event; the branch deletion (when not - ;; kept) already happened server-side. + ;; `:file-merged` event; the branch deletion already + ;; happened server-side. :merged (rx/of (ntf/success (tr "workspace.branches.merge.success")) (show-merge-result source-file-id)) :conflicts (rx/of (open-conflict-resolutions {:branch branch :mode :merge :conflicts conflicts :main-revn (:main-revn answer) - :excluded excluded})) + :excluded excluded + :comment comment})) :unsupported (rx/of (ntf/warn (tr "workspace.branches.merge.unsupported"))) (rx/of (ntf/error (tr "workspace.branches.merge.error")))))) (rx/catch diff --git a/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs b/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs index c3f17bce70..d287ff53ac 100644 --- a/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs +++ b/frontend/src/app/main/ui/ds/foundations/assets/icon.cljs @@ -155,11 +155,16 @@ (def ^:icon-id gap-horizontal "gap-horizontal") (def ^:icon-id gap-vertical "gap-vertical") (def ^:icon-id git-branch "git-branch") +(def ^:icon-id git-branch-minus "git-branch-minus") (def ^:icon-id git-branch-plus "git-branch-plus") (def ^:icon-id git-commit "git-commit") (def ^:icon-id git-commit-vertical "git-commit-vertical") +(def ^:icon-id git-compare-arrows "git-compare-arrows") +(def ^:icon-id git-graph "git-graph") (def ^:icon-id git-merge "git-merge") +(def ^:icon-id git-merge-conflict "git-merge-conflict") (def ^:icon-id git-pull-request-arrow "git-pull-request-arrow") +(def ^:icon-id git-pull-request-create-arrow "git-pull-request-create-arrow") (def ^:icon-id graphics "graphics") (def ^:icon-id grid "grid") (def ^:icon-id grid-column "grid-column") diff --git a/frontend/src/app/main/ui/ds/product/branch_row.cljs b/frontend/src/app/main/ui/ds/product/branch_row.cljs index 4e850fb875..a44a6cc3fb 100644 --- a/frontend/src/app/main/ui/ds/product/branch_row.cljs +++ b/frontend/src/app/main/ui/ds/product/branch_row.cljs @@ -23,23 +23,47 @@ (mf/defc branch-row* "A branch in a short list, as a button that opens it. The current one is marked and cannot be opened again. `children` go to the end of the row: - usually a `branch-status*`." + usually a `branch-status*`. A label cut short by the row's width shows + in full on hover." {::mf/schema schema:branch-row} [{:keys [class label is-current children] :rest props}] - (let [is-current (d/nilv is-current false) - props (mf/spread-props props - {:class [class (stl/css-case :branch-row true - :is-current is-current)] - :type "button" - :disabled is-current - :aria-current (when is-current "true")})] + (let [is-current (d/nilv is-current false) + + label-ref (mf/use-ref nil) + truncated* (mf/use-state false) + truncated? (deref truncated*) + + check-truncated + (mf/use-fn + (fn [] + (when-let [node (mf/ref-val label-ref)] + (reset! truncated* (> (.-scrollWidth node) (.-clientWidth node)))))) + + ;; the title goes on the button, not the label: the current row is + ;; disabled and still has to show it + props (mf/spread-props props + {:class [class (stl/css-case :branch-row true + :is-current is-current)] + :type "button" + :title (when truncated? label) + :disabled is-current + :aria-current (when is-current "true")})] + + (mf/with-effect [label] + (check-truncated) + (when-let [node (mf/ref-val label-ref)] + (let [observer (js/ResizeObserver. check-truncated)] + (.observe observer node) + #(.disconnect observer)))) + [:> :button props [:> icon* {:icon-id (if is-current i/tick i/git-branch) :size "s" :class (stl/css :icon)}] [:> text* {:as "span" :typography t/body-small - :class (stl/css :label)} + :class (stl/css :label) + :ref label-ref} label] (when (some? children) [:span {:class (stl/css :aside)} diff --git a/frontend/src/app/main/ui/ds/product/branch_row.mdx b/frontend/src/app/main/ui/ds/product/branch_row.mdx index c4813c0ced..b2657fe781 100644 --- a/frontend/src/app/main/ui/ds/product/branch_row.mdx +++ b/frontend/src/app/main/ui/ds/product/branch_row.mdx @@ -31,6 +31,10 @@ Children go to the end of the row, usually a `BranchStatus`. The label is mandatory. The row is a `button`; any other prop (`on-click`, `data-*`) goes to it. +A label too long for the row ends in an ellipsis, and the row then +carries the full label as its `title`, so it shows on hover (the current +row too, though it is disabled). A label that fits gets no `title`. + ```clj [:> branch-row* {:label (:name branch) :data-branch (dm/str (:branch-file-id branch)) diff --git a/frontend/src/app/main/ui/workspace/branch_switcher.cljs b/frontend/src/app/main/ui/workspace/branch_switcher.cljs index 9cbf08a4cb..d652af2a57 100644 --- a/frontend/src/app/main/ui/workspace/branch_switcher.cljs +++ b/frontend/src/app/main/ui/workspace/branch_switcher.cljs @@ -29,6 +29,8 @@ [app.main.ui.ds.product.branch-status :refer [branch-status*]] [app.main.ui.workspace.menu-shortcuts :refer [shortcuts*]] [app.main.ui.workspace.sidebar.pull-requests :as prui] + ;; registers the reviews dialog the popover opens + [app.main.ui.workspace.sidebar.reviews] [app.util.dom :as dom] [app.util.i18n :refer [tr]] [cuerdas.core :as str] @@ -84,14 +86,6 @@ (zero? (or (:behind row) 0)) (zero? (or (:conflicts row) 0)))) -(defn- main-subtitle - "The note beside main. Files that use a shared library read its main, - so on a library it says so." - [library?] - (if library? - (tr "workspace.branches.main-subtitle-library") - (tr "workspace.branches.main-subtitle"))) - ;; --- Popover (mf/defc switcher-row* @@ -104,10 +98,17 @@ :on-click on-open} ;; merged and archived branches carry no change counts (cond - (:is-main row) (main-subtitle (:is-library row)) + (:is-main row) (tr "workspace.branches.main-subtitle") (= "merged" (:status row)) (tr "workspace.branches.header.merged") (= "archived" (:status row)) (tr "workspace.branches.header.archived") - :else [:> row-status* {:row row :pr pr}])]]) + :else [:> row-status* {:row row :pr pr}])] + ;; files that use a shared library read its main: said under the row, + ;; so the note never takes the room of the branch name + (when (and (:is-main row) (:is-library row)) + [:> text* {:as "div" + :typography t/body-small + :class (stl/css :row-note)} + (tr "workspace.branches.main-library-note")])]) (mf/defc closed-group* "A collapsed section of the popover for branches out of the flow: its @@ -134,7 +135,7 @@ (mf/defc switcher-popover* {::mf/private true} - [{:keys [file current prs-by-branch on-close]}] + [{:keys [current prs-by-branch on-close]}] (let [{:keys [data status]} (mf/deref branches) filter* (mf/use-state "") @@ -177,10 +178,17 @@ on-create (mf/use-fn - (mf/deps file on-close) + (mf/deps on-close) (fn [_] (on-close) - (modal/show! :create-branch {:file-name (:name file)}))) + (modal/show! :create-branch {}))) + + on-show-reviews + (mf/use-fn + (mf/deps on-close) + (fn [_] + (on-close) + (modal/show! :branch-reviews {}))) on-show-all (mf/use-fn @@ -251,9 +259,6 @@ :pr (get prs-by-branch (:id row)) :on-open on-open}])]]) - [:> closed-group* {:label (tr "workspace.branches.merged") - :rows (with-status "merged") - :on-open on-open}] [:> closed-group* {:label (tr "workspace.branches.archived") :rows (with-status "archived") :on-open on-open}]]) @@ -262,12 +267,18 @@ ;; branches only grow from main: on a branch there is nothing to create (when (:is-main current) - [:button {:class (stl/css :action) :on-click on-create} - [:> i/icon* {:icon-id i/git-branch-plus :size "s"}] - [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.new")]]) + [:* + [:button {:class (stl/css :action) :on-click on-create} + [:> i/icon* {:icon-id i/git-branch-plus :size "s"}] + [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.new")]] + [:hr {:class (stl/css :divider)}]]) + + [:button {:class (stl/css :action) :on-click on-show-reviews} + [:> i/icon* {:icon-id i/git-pull-request-arrow :size "s"}] + [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.reviews")]] [:button {:class (stl/css :action) :on-click on-show-all} - [:> i/icon* {:icon-id i/menu :size "s"}] + [:> i/icon* {:icon-id i/git-graph :size "s"}] [:span {:class (stl/css :action-label)} (tr "workspace.branches.switcher.all")] [:> shortcuts* {:id :open-branches}]]])) @@ -365,7 +376,7 @@ [:> text* {:as "span" :typography t/body-small :class (stl/css :status-note)} - (main-subtitle (:is-shared file))] + (tr "workspace.branches.main-subtitle")] ;; merged and archived branches open read-only and count no ;; changes: the note says which of the two it is @@ -391,7 +402,6 @@ [:> row-status* {:row ctx :pr pr}])) [:& dropdown {:show open? :on-close on-close :container bar-ref} - [:> switcher-popover* {:file file - :current current + [:> switcher-popover* {:current current :prs-by-branch prs-by-branch :on-close on-close}]]]))) diff --git a/frontend/src/app/main/ui/workspace/branch_switcher.scss b/frontend/src/app/main/ui/workspace/branch_switcher.scss index 8f7a794cab..8a7466fc13 100644 --- a/frontend/src/app/main/ui/workspace/branch_switcher.scss +++ b/frontend/src/app/main/ui/workspace/branch_switcher.scss @@ -24,8 +24,12 @@ padding: var(--sp-xs) var(--sp-m) var(--sp-s); } +// the note gives way before the branch name in the chip does .status-note { - flex-shrink: 0; + min-inline-size: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; color: var(--color-foreground-secondary); } @@ -71,6 +75,15 @@ list-style: none; } +// lined up with the end of the row above, where its subtitle sits +.row-note { + padding-block-end: var(--sp-xs); + padding-inline: var(--sp-s); + color: var(--color-foreground-secondary); + font-style: italic; + text-align: end; +} + .list-note { padding: var(--sp-s); color: var(--color-foreground-secondary); diff --git a/frontend/src/app/main/ui/workspace/main_menu.cljs b/frontend/src/app/main/ui/workspace/main_menu.cljs index e9a4e2b837..8809a2ebc3 100644 --- a/frontend/src/app/main/ui/workspace/main_menu.cljs +++ b/frontend/src/app/main/ui/workspace/main_menu.cljs @@ -968,8 +968,8 @@ ::ev/origin "workspace:menu"}) (modal/show :plugin-management {})))) - ;; the switcher when the file has branches; the create dialog when - ;; it has none, since the switcher only exists once a branch does + ;; the branches dialog when the file has branches; the create + ;; dialog when it has none show-branches (mf/use-fn (fn [event] diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs index d2d3c0b908..cb1b533c72 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs @@ -22,7 +22,7 @@ [app.main.ui.ds.controls.checkbox :refer [checkbox*]] [app.main.ui.ds.controls.input :refer [input*]] [app.main.ui.ds.foundations.assets.icon :as i] - [app.main.ui.ds.layout.menu :refer [menu* menu-item*]] + [app.main.ui.ds.layout.menu :refer [menu* menu-item* menu-separator*]] [app.main.ui.ds.layout.modal :refer [modal* modal-header* modal-content* modal-footer*]] [app.main.ui.ds.layout.tab-switcher :refer [tab-switcher*]] [app.main.ui.ds.notifications.context-notification :refer [context-notification*]] @@ -232,7 +232,7 @@ (mf/defc create-branch-dialog* {::mf/register modal/components ::mf/register-as :create-branch} - [{:keys [file-name]}] + [] (let [;; a branch only grows from main, so the open file is main library? (boolean (:is-shared (mf/deref refs/file))) branch-name* (mf/use-state "") @@ -261,8 +261,7 @@ [:> modal* {:is-open true :on-open-change (prui/on-dismiss on-close)} [:> prui/dialog-header* {:icon i/git-branch - :title (tr "workspace.branches.create.title") - :subtitle (tr "workspace.branches.create.subtitle" (or file-name ""))}] + :title (tr "workspace.branches.create.title")}] [:> modal-content* {:class (stl/css :dialog-content)} [:> input* {:label (tr "workspace.branches.create.name-label") @@ -303,6 +302,64 @@ :on-click on-submit} (tr "workspace.branches.create.submit")]]])) +;; --- Rename branch dialog (modal) +;; +;; The branch list renames in place; the bottom bar has no room for that, +;; so it asks for the new name here. + +(mf/defc rename-branch-dialog* + {::mf/register modal/components + ::mf/register-as :rename-branch} + [{:keys [branch]}] + (let [branch-name* (mf/use-state (or (:name branch) "")) + branch-name (deref branch-name*) + value (str/trim branch-name) + valid? (and (not (str/blank? value)) + (not= value (:name branch))) + + on-name-change + (mf/use-fn #(reset! branch-name* (dom/get-target-val %))) + + on-close + (mf/use-fn #(st/emit! (modal/hide))) + + on-submit + (mf/use-fn + (mf/deps branch value valid?) + (fn [_] + (when valid? + (st/emit! (dwb/rename-branch (:id branch) value) + (modal/hide))))) + + on-key-down + (mf/use-fn + (mf/deps on-submit) + (fn [event] + (when (kbd/enter? event) + (on-submit event))))] + + [:> modal* {:is-open true + :on-open-change (prui/on-dismiss on-close)} + [:> prui/dialog-header* {:icon i/git-branch + :title (tr "workspace.branches.rename.title") + :subtitle (:name branch)}] + + [:> modal-content* {:class (stl/css :dialog-content)} + [:> input* {:label (tr "workspace.branches.create.name-label") + :icon i/git-branch + :value branch-name + :auto-focus true + :on-change on-name-change + :on-key-down on-key-down}]] + + [:> modal-footer* {} + [:> button* {:variant "secondary" :on-click on-close} + (tr "labels.cancel")] + [:> button* {:variant "primary" + :disabled (not valid?) + :on-click on-submit} + (tr "labels.rename")]]])) + ;; --- Preview limits dialog (modal) ;; ;; Every refusal the branching commands can emit has a line here, so a user @@ -381,8 +438,8 @@ {::mf/register modal/components ::mf/register-as :merge-branch} [{:keys [branch excluded]}] - (let [archive?* (mf/use-state false) - archive? (deref archive?*) + (let [comment* (mf/use-state "") + comment (deref comment*) ;; The branch name and "main" are highlighted (foreground-primary) ;; within the otherwise secondary-colored prompt. We wrap each of @@ -396,17 +453,17 @@ (str sep (tr "workspace.branches.main") sep))] (str/split msg sep)) - on-toggle-archive - (mf/use-fn #(swap! archive?* not)) + on-comment-change + (mf/use-fn #(reset! comment* (dom/get-target-val %))) on-close (mf/use-fn #(st/emit! (modal/hide))) on-submit (mf/use-fn - (mf/deps branch archive? excluded) + (mf/deps branch comment excluded) (fn [_] - (st/emit! (dwb/merge-branch branch {:keep-branch archive? :excluded excluded}) + (st/emit! (dwb/merge-branch branch {:comment comment :excluded excluded}) (modal/hide))))] [:> modal* {:is-open true @@ -420,16 +477,20 @@ [:span {:key idx :class (stl/css :modal-message-emphasis)} part] [:span {:key idx} part]))] - [:> checkbox* {:id "merge-archive-branch" - :label (tr "workspace.branches.merge.archive-label") - :checked archive? - :on-change on-toggle-archive}] + ;; kept with the record of the merge, in the reviews dialog + [:div {:class (stl/css :field)} + [:label {:class (stl/css :field-label) + :for "merge-branch-comment"} + (tr "workspace.branches.merge.comment-label")] + [:textarea {:id "merge-branch-comment" + :class (stl/css :textarea) + :value comment + :max-length 4000 + :on-change on-comment-change + :rows 3}]] - (if archive? - [:> context-notification* {:level :warning :type :context} - (tr "workspace.branches.merge.archive-warning")] - [:> context-notification* {:level :info :type :context} - (tr "workspace.branches.merge.delete-info")])] + [:> context-notification* {:level :info :type :context} + (tr "workspace.branches.merge.delete-info")]] [:> modal-footer* {} [:> button* {:variant "secondary" :on-click on-close} @@ -442,8 +503,8 @@ ;; --- Confirmation for relevant branch actions (merge / update / materialize) (defn- confirm-merge! - "Open the merge dialog for `branch` (merge into main is irreversible; the - dialog also lets the user choose whether to keep the branch archived). + "Open the merge dialog for `branch` (merge into main is irreversible and + deletes the branch; the dialog asks for a comment for its record). `excluded` is the compare dialog's per-change selection, if any." ([branch] (confirm-merge! branch nil)) ([branch excluded] @@ -474,6 +535,45 @@ :accept-style :danger :on-accept (fn [_] (st/emit! (dwb/materialize-branch (:branch-file-id branch))))}))) +(defn- confirm-archive! + "Ask for confirmation before archiving `branch`, or restoring it with + `archived?` false. The confirmation takes the place of any open dialog." + [branch archived?] + (st/emit! (modal/show {:type :confirm + :title (if archived? + (tr "workspace.branches.archive.title") + (tr "workspace.branches.restore.title")) + :message (if archived? + (tr "workspace.branches.archive.message" (:name branch)) + (tr "workspace.branches.restore.message" (:name branch))) + :accept-label (if archived? + (tr "workspace.branches.menu.archive") + (tr "workspace.branches.menu.restore")) + :accept-style :primary + :on-accept (fn [_] (st/emit! (dwb/archive-branch (:id branch) archived?)))}))) + +(defn- confirm-delete! + "Ask for confirmation before deleting `branch`. `open-file-id`, when + given, is the file to land on afterwards: the one the open branch comes + from, because deleting it takes the open file away." + ([branch] (confirm-delete! branch nil)) + ([branch open-file-id] + (st/emit! (modal/show {:type :confirm + :title (tr "workspace.branches.delete.title") + :message (tr "workspace.branches.delete.message" (:name branch)) + :accept-label (tr "labels.delete") + :accept-style :danger + :on-accept (fn [_] (st/emit! (dwb/delete-branch (:id branch) open-file-id)))})))) + +;; --- Profiles shown as avatars + +(defn- avatar-profile + "The fields of a profile an avatar reads. The logged-in profile carries + more, such as a `:theme` that is nil until the user picks one, which the + avatar's schema refuses (it takes a string or nothing)." + [profile] + (select-keys profile [:id :fullname :email :photo-id :photo-url])) + ;; --- Diff counts: known, or honestly unknown ;; ;; A row whose comparison could not be made carries no counts at all (and @@ -495,7 +595,7 @@ (let [profile (mf/deref refs/profile) profiles (mf/deref refs/profiles) author (or (get profiles (:created-by branch)) - (when (= (:created-by branch) (:id profile)) profile)) + (when (= (:created-by branch) (:id profile)) (avatar-profile profile))) author? (= (:created-by branch) (:id profile)) status (:status branch) merged? (= "merged" status) @@ -521,9 +621,7 @@ on-compare (mf/use-fn (mf/deps branch) #(modal/show! :branch-compare {:branch branch})) on-restore (mf/use-fn (mf/deps branch) - (fn [_] - (st/emit! (modal/hide) - (dwb/archive-branch (:id branch) false)))) + #(confirm-archive! branch false)) on-materialize (mf/use-fn (mf/deps branch) #(confirm-materialize! branch))] [:> modal* {:is-open true @@ -613,20 +711,24 @@ [:dd {:class (stl/css :info-val)} (-> (:merged-at branch) ct/inst (ct/format-inst :localized-date-time))]])]] + ;; the dialog closes from its header; the footer only acts on the + ;; branch. The work of a merged branch is in main: there is nothing + ;; to compare [:> modal-footer* {} - ;; an archived branch is read-only: it comes back to work restored, - ;; or leaves the branch engine as an ordinary file - (when archived? - [:> button* {:variant "secondary" :icon i/history :on-click on-restore} - (tr "workspace.branches.menu.restore")]) - (when archived? - [:> button* {:variant "secondary" :icon i/document :on-click on-materialize} - (tr "workspace.branches.materialize.action")]) - ;; the work of a merged branch is in main: there is nothing to compare (when-not merged? - [:> button* {:variant "secondary" :icon i/switch :on-click on-compare} + [:> button* {:variant (if archived? "secondary" "primary") + :icon i/git-compare-arrows + :on-click on-compare} (tr "workspace.branches.compare")]) - [:> button* {:variant "primary" :on-click on-close} (tr "labels.close")]]])) + ;; an archived branch is read-only: it leaves the branch engine as an + ;; ordinary file, or comes back to work restored, its primary action. + ;; The buttons follow the bottom bar of an open archived branch + (when archived? + [:* + [:> button* {:variant "secondary" :icon i/git-branch-minus :on-click on-materialize} + (tr "workspace.branches.materialize.action")] + [:> button* {:variant "primary" :icon i/history :on-click on-restore} + (tr "workspace.branches.menu.restore")]])]])) ;; --- Branch list entry @@ -769,7 +871,7 @@ (fn [event] (dom/stop-propagation event) (on-set-menu false) - (st/emit! (dwb/archive-branch (:id entry) (not archived?))))) + (confirm-archive! entry (not archived?)))) on-delete (mf/use-fn @@ -777,12 +879,7 @@ (fn [event] (dom/stop-propagation event) (on-set-menu false) - (st/emit! (modal/show {:type :confirm - :title (tr "workspace.branches.delete.title") - :message (tr "workspace.branches.delete.message" (:name entry)) - :accept-label (tr "labels.delete") - :accept-style :danger - :on-accept (fn [_] (st/emit! (dwb/delete-branch (:id entry))))})))) + (confirm-delete! entry))) ;; the exit door: the branch stops being a branch but keeps its ;; content, and the confirmation says so, because the branch row and @@ -908,6 +1005,7 @@ (when-not merged? [:> menu-item* {:id "materialize" :on-action on-materialize} (tr "workspace.branches.materialize.action")]) + [:> menu-separator* {}] [:> menu-item* {:id "delete" :on-action on-delete} (tr "labels.delete")]]])]])) @@ -915,12 +1013,11 @@ (defn- branches-for-tab "The branch rows a tab of the all-branches dialog lists: the open ones, - the open ones the profile created, the merged ones, or the archived - ones." + the open ones the profile created, or the archived ones. A merge deletes + its branch, so no tab lists merged branches." [tab rows profile-id] (case tab "mine" (filter #(and (= "open" (:status %)) (= profile-id (:created-by %))) rows) - "merged" (filter #(= "merged" (:status %)) rows) "archived" (filter #(= "archived" (:status %)) rows) (filter #(= "open" (:status %)) rows))) @@ -937,7 +1034,7 @@ profile-id (:id profile) profiles (cond-> profiles - profile-id (assoc profile-id profile)) + profile-id (assoc profile-id (avatar-profile profile))) prs-by-branch (mf/with-memo [prs] @@ -955,7 +1052,16 @@ ;; only one entry's options dropdown may be open at a time open-menu* (mf/use-state nil) open-menu (deref open-menu*) - set-menu (mf/use-fn (fn [k open?] (reset! open-menu* (when open? k)))) + ;; the DS menu closes every other open menu when one opens, and + ;; each of them answers with `open?` false: closing one entry's + ;; menu must not clear the menu another entry just opened + set-menu (mf/use-fn + (fn [k open?] + (swap! open-menu* (fn [current] + (cond + open? k + (= current k) nil + :else current))))) matching (mf/with-memo [data filter-v] @@ -968,7 +1074,6 @@ (let [label (fn [text t] (dm/str text " (" (count (branches-for-tab t data profile-id)) ")"))] [{:id "mine" :label (label (tr "workspace.branches.list.tab.mine") "mine")} {:id "active" :label (label (tr "workspace.branches.list.tab.active") "active")} - {:id "merged" :label (label (tr "workspace.branches.merged") "merged")} {:id "archived" :label (label (tr "workspace.branches.archived") "archived")}])) rows (branches-for-tab tab matching profile-id) @@ -976,14 +1081,16 @@ on-tab-change (mf/use-fn #(reset! tab* %)) on-filter-change (mf/use-fn #(reset! filter* (dom/get-target-val %))) + ;; the input's wrapper takes the full width it is given: this + ;; box sets that width, so the tabs keep room for their labels search (mf/html - [:> input* {:variant "dense" - :icon i/search - :class (stl/css :branches-list-search) - :placeholder (tr "workspace.branches.search.placeholder") - :value filter-v - :on-change on-filter-change}]) + [:div {:class (stl/css :branches-list-search)} + [:> input* {:variant "dense" + :icon i/search + :placeholder (tr "workspace.branches.search.placeholder") + :value filter-v + :on-change on-filter-change}]]) on-open-change (mf/use-fn @@ -995,10 +1102,7 @@ (mf/use-fn #(modal/show! :branch-limits {})) on-create - (mf/use-fn - (mf/deps file) - (fn [_] - (modal/show! :create-branch {:file-name (:name file)}))) + (mf/use-fn #(modal/show! :create-branch {})) on-retry (mf/use-fn #(st/emit! (dwb/init-branches-state)))] @@ -1186,7 +1290,7 @@ (cond (nil? item) [:div {:class (stl/css :compare-detail-empty)} - [:> empty-state* {:icon i/switch + [:> empty-state* {:icon i/git-compare-arrows :text (tr "workspace.branches.compare.select-hint")}]] :else @@ -1291,7 +1395,7 @@ :on-open-change (prui/on-dismiss on-close) :size "xlarge"} [:> prui/dialog-header* - {:icon i/switch + {:icon i/git-compare-arrows :title (tr "workspace.branches.compare.title") :subtitle (mf/html @@ -1332,7 +1436,7 @@ (cond (= status :loading) [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/switch + [:> empty-state* {:icon i/git-compare-arrows :text (tr "workspace.branches.compare.loading")}]] (= status :error) @@ -1342,7 +1446,7 @@ (empty? items) [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/git-branch + [:> empty-state* {:icon i/git-compare-arrows :text (tr "workspace.branches.compare.empty")}]] :else @@ -1428,7 +1532,7 @@ (cond conflicts? [:> button* {:variant "primary" - :icon i/git-merge + :icon i/git-merge-conflict :on-click on-resolve} (tr "workspace.branches.conflicts.resolve")] @@ -1651,16 +1755,11 @@ (mf/defc branch-conflicts-dialog* {::mf/register modal/components ::mf/register-as :branch-conflicts} - [{:keys [branch mode conflicts excluded]}] + [{:keys [branch mode conflicts excluded comment]}] (let [{:keys [diff selected resolutions status]} (mf/deref branch-diff) loading? (= status :loading) - ;; merge mode only: whether to keep the branch archived after merge - keep?* (mf/use-state false) - keep? (deref keep?*) - on-toggle-keep (mf/use-fn #(swap! keep?* not)) - diff-meta (:meta diff) ;; per-side header subtitle: base is pinned at branch creation, main ;; and branch show how recent each one is ("3 hours ago"); the branch @@ -1705,12 +1804,13 @@ #(when sel (st/emit! (dwb/set-conflict-resolution (:id sel) :main)))) on-use-branch (mf/use-fn (mf/deps sel) #(when sel (st/emit! (dwb/set-conflict-resolution (:id sel) :branch)))) - on-apply (mf/use-fn (mf/deps branch resolutions mode keep? excluded) + ;; a merge sends again the comment of the merge dialog + on-apply (mf/use-fn (mf/deps branch resolutions mode excluded comment) #(st/emit! (if (= mode :update) (dwb/update-branch-from-main branch {:resolutions resolutions :excluded excluded}) (dwb/merge-branch branch {:resolutions resolutions - :keep-branch keep? + :comment comment :excluded excluded}))))] ;; the fetched diff backs both the fallback conflict set and the side @@ -1724,7 +1824,7 @@ [:> modal* {:is-open true :on-open-change (prui/on-dismiss on-close) :size "xlarge"} - [:> prui/dialog-header* {:icon i/triangle-alert + [:> prui/dialog-header* {:icon i/git-merge-conflict :tone :warning :title (tr "workspace.branches.conflicts.title") :subtitle (tr "workspace.branches.conflicts.subtitle" (:name branch))} @@ -1747,12 +1847,12 @@ (cond loading? [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/git-merge + [:> empty-state* {:icon i/git-merge-conflict :text (tr "workspace.branches.compare.loading")}]] (empty? conflicts) [:div {:class (stl/css :compare-empty)} - [:> empty-state* {:icon i/git-merge + [:> empty-state* {:icon i/git-merge-conflict :text (tr "workspace.branches.conflicts.empty")}]] :else @@ -1837,13 +1937,6 @@ :end (mf/html [:* - ;; applying a MERGE deletes the branch unless the user opts to - ;; keep it archived — same choice the plain merge dialog offers - (when (not= mode :update) - [:> checkbox* {:id "conflicts-archive-branch" - :label (tr "workspace.branches.merge.archive-label") - :checked keep? - :on-change on-toggle-keep}]) [:> button* {:variant "secondary" :on-click on-close} (tr "labels.cancel")] [:> button* {:variant "primary" :icon i/git-merge @@ -1945,6 +2038,27 @@ (= (:id ctx) (:file-branch-id %)))) (first))) + profile (mf/deref refs/profile) + team (mf/deref refs/team) + pr-author? (and (some? open-pr) (= (:created-by open-pr) (:id profile))) + admin? (boolean (get-in team [:permissions :is-admin])) + can-merge? (boolean (get-in team [:permissions :can-edit])) + + open? (not (or merged? archived?)) + ahead (or (:ahead ctx) 0) + behind (or (:behind ctx) 0) + + ;; the one action an open branch offers as its primary button, most + ;; pressing first; the options menu leaves it out + primary + (when open? + (cond + (pos? (or (:conflicts ctx) 0)) :resolve + (pos? behind) :update + (and prs-enabled? (some? open-pr)) :open-review + (and prs-enabled? (pos? ahead)) :request-review + (pos? ahead) :merge)) + menu-open* (mf/use-state false) menu-open? (deref menu-open*) on-toggle-menu (mf/use-fn #(swap! menu-open* not)) @@ -1953,7 +2067,7 @@ on-compare (mf/use-fn (mf/deps ctx) #(modal/show! :branch-compare {:branch ctx})) on-update (mf/use-fn (mf/deps ctx) #(confirm-update! ctx)) on-open-main (mf/use-fn (mf/deps ctx) #(st/emit! (dwb/open-branch (:source-file-id ctx)))) - on-restore (mf/use-fn (mf/deps ctx) #(st/emit! (dwb/archive-branch (:id ctx) false))) + on-restore (mf/use-fn (mf/deps ctx) #(confirm-archive! ctx false)) on-materialize (mf/use-fn (mf/deps ctx) #(confirm-materialize! ctx)) on-resolve (mf/use-fn (mf/deps ctx) #(modal/show! :branch-conflicts {:branch ctx :mode :merge})) @@ -1974,6 +2088,27 @@ (mf/deps open-pr) #(st/emit! (dwpr/open-pull-request open-pr))) + ;; the same options as the branch's entry in the branch list; + ;; deleting the open branch lands on the file it comes from + on-menu-action + (mf/use-fn + (mf/deps ctx open-pr) + (fn [action] + (case action + "info" (modal/show! :branch-info {:branch ctx}) + "update" (confirm-update! ctx) + "request-review" (modal/show! :create-pull-request {:branch ctx}) + "open-review" (st/emit! (dwpr/open-pull-request open-pr)) + "review-details" (modal/show! :pull-request-info {:pr open-pr}) + "publish-review" (st/emit! (dwpr/update-pull-request-snapshot (:id open-pr))) + "merge" (confirm-merge! ctx) + "cancel-review" (prui/confirm-close! open-pr) + "rename" (modal/show! :rename-branch {:branch ctx}) + "archive" (confirm-archive! ctx true) + "materialize" (confirm-materialize! ctx) + "delete" (confirm-delete! ctx (:source-file-id ctx)) + nil))) + ;; publishes the room the banner takes from the bottom of the canvas ;; whenever its box changes size, which is what keeps the floating ;; colour/typography palette bar clear of it in both the collapsed and @@ -2041,85 +2176,122 @@ [:span {:class (stl/css :branch-banner-title)} (:label status)]] [:div {:class (stl/css :branch-banner-separator)}] - (cond - merged? - [:> button* {:variant "secondary" - :icon i/arrow-up-right - :on-click on-open-main} - (tr "workspace.branches.banner.view-main")] - - ;; an archived branch was set aside unmerged and is read-only: it - ;; can be compared with main, brought back to work, or taken out - ;; as an ordinary file - archived? - [:div {:class (stl/css :branch-banner-actions)} + [:div {:class (stl/css :branch-banner-actions)} + (cond + merged? [:> button* {:variant "secondary" - :icon i/switch - :on-click on-compare} - (tr "workspace.branches.compare")] - [:> button* {:variant "secondary" - :icon i/document - :on-click on-materialize} - (tr "workspace.branches.materialize.action")] - [:> button* {:variant "primary" - :icon i/history - :on-click on-restore} - (tr "workspace.branches.menu.restore")]] + :icon i/arrow-up-right + :on-click on-open-main} + (tr "workspace.branches.banner.view-main")] - :else - (let [conflicts (or (:conflicts ctx) 0)] - [:div {:class (stl/css :branch-banner-actions)} - [:> button* {:variant "secondary" - :icon i/switch - :on-click on-compare} - (tr "workspace.branches.compare")] + ;; an archived branch was set aside unmerged and is read-only: it + ;; can be compared with main, brought back to work, or taken out + ;; as an ordinary file + archived? + [:* + [:> button* {:variant "secondary" + :icon i/git-compare-arrows + :on-click on-compare} + (tr "workspace.branches.compare")] + [:> button* {:variant "secondary" + :icon i/git-branch-minus + :on-click on-materialize} + (tr "workspace.branches.materialize.action")] + [:> button* {:variant "primary" + :icon i/history + :on-click on-restore} + (tr "workspace.branches.menu.restore")]] - (cond - (pos? conflicts) - [:> button* {:variant "primary" - :icon i/triangle-alert - :on-click on-resolve} - (tr "workspace.branches.conflicts.resolve")] + :else + [:* + [:> button* {:variant "secondary" + :icon i/git-compare-arrows + :on-click on-compare} + (tr "workspace.branches.compare")] - (pos? (or (:behind ctx) 0)) - [:> button* {:variant "primary" - :icon i/status-update - :on-click on-update} - (tr "workspace.branches.update")] + ;; with pull requests enabled the primary flow is asking for a + ;; review (merging moves to the options menu) + (case primary + :resolve + [:> button* {:variant "primary" + :icon i/git-merge-conflict + :on-click on-resolve} + (tr "workspace.branches.conflicts.resolve")] - ;; with pull requests enabled the primary flow is asking - ;; for a review (merging moves behind the kebab menu) - (and prs-enabled? (some? open-pr)) - [:> button* {:variant "primary" - :icon i/git-pull-request-arrow - :on-click on-open-review} - (tr "workspace.pull-requests.actions.open-review")] + :update + [:> button* {:variant "primary" + :icon i/status-update + :on-click on-update} + (tr "workspace.branches.update")] - (and prs-enabled? (pos? (or (:ahead ctx) 0))) - [:> button* {:variant "primary" - :icon i/git-pull-request-arrow - :on-click on-request-review} - (tr "workspace.pull-requests.menu.request-review")] + :open-review + [:> button* {:variant "primary" + :icon i/git-pull-request-arrow + :on-click on-open-review} + (tr "workspace.pull-requests.actions.open-review")] - (pos? (or (:ahead ctx) 0)) - [:> button* {:variant "primary" - :icon i/git-merge - :on-click on-merge} - (tr "workspace.branches.merge.action")]) + :request-review + [:> button* {:variant "primary" + :icon i/git-pull-request-create-arrow + :on-click on-request-review} + (tr "workspace.pull-requests.menu.request-review")] - (when prs-enabled? - ;; the banner sits at the bottom of the canvas: its menu - ;; opens upwards - [:div {:class (stl/css :branch-banner-menu)} - [:> menu* {:is-open menu-open? - :on-open-change on-menu-change - :placement "top end" - :trigger - (mf/html - [:> icon-button* {:variant "ghost" - :icon i/menu - :aria-label (tr "labels.options") - :aria-pressed menu-open? - :on-click on-toggle-menu}])} - [:> menu-item* {:id "merge" :on-action on-merge} - (tr "workspace.pull-requests.actions.merge")]]])]))]]))) + :merge + [:> button* {:variant "primary" + :icon i/git-merge + :on-click on-merge} + (tr "workspace.branches.merge.action")] + + nil)]) + + ;; the branch's options, minus what the banner already offers as + ;; a button; the banner sits at the bottom of the canvas, so the + ;; menu opens upwards + [:div {:class (stl/css :branch-banner-menu)} + [:> menu* {:is-open menu-open? + :on-open-change on-menu-change + :on-action on-menu-action + :placement "top end" + :trigger + (mf/html + [:> icon-button* {:variant "ghost" + :icon i/menu + :aria-label (tr "labels.options") + :aria-pressed menu-open? + :on-click on-toggle-menu}])} + [:> menu-item* {:id "info"} + (tr "workspace.branches.menu.info")] + (when (and open? (pos? behind) (not= primary :update)) + [:> menu-item* {:id "update"} + (tr "workspace.branches.update")]) + (when (and open? prs-enabled? (nil? open-pr) (not= primary :request-review)) + [:> menu-item* {:id "request-review"} + (tr "workspace.pull-requests.menu.request-review")]) + (when (and (some? open-pr) (not= primary :open-review)) + [:> menu-item* {:id "open-review"} + (tr "workspace.pull-requests.actions.open-review")]) + (when (some? open-pr) + [:> menu-item* {:id "review-details"} + (tr "workspace.pull-requests.menu.review-details")]) + (when pr-author? + [:> menu-item* {:id "publish-review"} + (tr "workspace.pull-requests.menu.publish")]) + (when (and open? prs-enabled? (not= primary :merge) + (or (nil? open-pr) can-merge?)) + [:> menu-item* {:id "merge"} + (tr "workspace.pull-requests.actions.merge")]) + (when (and (some? open-pr) (or pr-author? admin?)) + [:> menu-item* {:id "cancel-review"} + (tr "workspace.pull-requests.actions.close")]) + (when open? + [:> menu-item* {:id "rename"} + (tr "labels.rename")]) + (when open? + [:> menu-item* {:id "archive"} + (tr "workspace.branches.menu.archive")]) + (when open? + [:> menu-item* {:id "materialize"} + (tr "workspace.branches.materialize.action")]) + [:> menu-separator* {}] + [:> menu-item* {:id "delete"} + (tr "labels.delete")]]]]]]))) diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.scss b/frontend/src/app/main/ui/workspace/sidebar/branches.scss index 41a3aa8715..ba94c39431 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.scss +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.scss @@ -14,6 +14,7 @@ .branches-empty { display: flex; flex-direction: column; + align-items: center; gap: var(--sp-xxxl); margin: var(--sp-xxxl) auto; inline-size: $sz-200; @@ -154,8 +155,8 @@ } // Merge-dialog confirmation prompt: a regular-weight, secondary-color -// paragraph sitting between the title and the archive controls, with a -// touch more breathing room before the checkbox. +// paragraph sitting between the title and the comment field, with a +// touch more breathing room before it. .modal-message { @include use-typography("body-large"); @@ -1112,7 +1113,7 @@ .branch-banner { position: absolute; - inset-block-end: var(--sp-m); + inset-block-end: var(--sp-xs); inset-inline-start: 50%; transform: translateX(-50%); max-inline-size: 90vw; @@ -1253,8 +1254,7 @@ } .branches-list-search { - inline-size: $sz-224; - margin-inline-start: auto; + inline-size: $sz-192; } .branches-list-entries { diff --git a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs index e511624e85..f9c485f97f 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs @@ -210,7 +210,7 @@ [:> modal* {:is-open true :on-open-change (on-dismiss on-close) :size "large"} - [:> dialog-header* {:icon i/git-pull-request-arrow + [:> dialog-header* {:icon i/git-pull-request-create-arrow :title (tr "workspace.pull-requests.create.title") :subtitle (tr "workspace.pull-requests.create.subtitle" (or (:name branch) ""))}] @@ -255,7 +255,7 @@ [:> button* {:variant "secondary" :on-click on-close} (tr "labels.cancel")] [:> button* {:variant "primary" - :icon i/git-pull-request-arrow + :icon i/git-pull-request-create-arrow :disabled (not valid?) :on-click on-submit} (tr "workspace.pull-requests.create.submit")]]])) @@ -608,7 +608,7 @@ [:div {:class (stl/css :pr-banner-actions)} [:> button* {:variant "secondary" - :icon i/switch + :icon i/git-compare-arrows :on-click on-compare} (tr "workspace.branches.compare")] diff --git a/frontend/src/app/main/ui/workspace/sidebar/reviews.cljs b/frontend/src/app/main/ui/workspace/sidebar/reviews.cljs new file mode 100644 index 0000000000..3289ad2c01 --- /dev/null +++ b/frontend/src/app/main/ui/workspace/sidebar/reviews.cljs @@ -0,0 +1,405 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns app.main.ui.workspace.sidebar.reviews + "The reviews dialog: the merges into main, each with who made it, when + and the comment left with it, and the design reviews (pull requests) of + the file. It follows the all-branches dialog: tabs with their counts, + a search box and one list per tab." + (:require-macros [app.main.style :as stl]) + (:require + [app.common.data.macros :as dm] + [app.common.time :as ct] + [app.main.data.modal :as modal] + [app.main.data.workspace.branches :as dwb] + [app.main.data.workspace.pull-requests :as dwpr] + [app.main.refs :as refs] + [app.main.store :as st] + [app.main.ui.ds.buttons.button :refer [button*]] + [app.main.ui.ds.buttons.icon-button :refer [icon-button*]] + [app.main.ui.ds.controls.input :refer [input*]] + [app.main.ui.ds.foundations.assets.icon :as i] + [app.main.ui.ds.layout.menu :refer [menu* menu-item*]] + [app.main.ui.ds.layout.modal :refer [modal* modal-header* modal-content*]] + [app.main.ui.ds.layout.tab-switcher :refer [tab-switcher*]] + [app.main.ui.ds.product.avatar :refer [avatar*]] + [app.main.ui.ds.product.branch-pill :refer [branch-pill*]] + [app.main.ui.ds.product.empty-state :refer [empty-state*]] + [app.main.ui.workspace.sidebar.pull-requests :as prui :refer [pr-state-badge*]] + [app.util.dom :as dom] + [app.util.i18n :refer [tr]] + [app.util.keyboard :as kbd] + [cuerdas.core :as str] + [okulary.core :as l] + [rumext.v2 :as mf])) + +(def ^:private merges + (l/derived :workspace-branch-merges st/state)) + +(defn- matches? + "Whether any of `values` contains the search text, ignoring case. An + empty search matches everything." + [filter-v values] + (or (str/blank? filter-v) + (let [needle (str/lower filter-v)] + (some #(str/includes? (str/lower (or % "")) needle) values)))) + +;; --- Shared pieces + +(mf/defc clamped-text* + "A text of any length shown on two lines at most. When it does not fit, + a \"See more\" link shows the rest and \"See less\" folds it back. The + link is a button of its own, so it never acts on the entry around it." + {::mf/private true} + [{:keys [text]}] + (let [text-ref (mf/use-ref nil) + expanded* (mf/use-state false) + expanded? (deref expanded*) + overflow* (mf/use-state false) + overflow? (deref overflow*) + + on-toggle + (mf/use-fn + (fn [event] + (dom/stop-propagation event) + (swap! expanded* not)))] + + ;; measured only while folded: unfolded, the text always fits + (mf/with-effect [text expanded?] + (when-not expanded? + (when-let [node (mf/ref-val text-ref)] + (let [measure #(reset! overflow* (> (.-scrollHeight node) (.-clientHeight node))) + observer (js/ResizeObserver. measure)] + (measure) + (.observe observer node) + #(.disconnect observer))))) + + [:div {:class (stl/css :clamped)} + [:p {:ref text-ref + :class (stl/css-case :clamped-text true + :is-folded (not expanded?))} + text] + (when (or overflow? expanded?) + [:button {:type "button" + :class (stl/css :see-more) + :aria-expanded expanded? + :on-click on-toggle} + (if expanded? + (tr "workspace.branches.reviews.see-less") + (tr "workspace.branches.reviews.see-more"))])])) + +(mf/defc entry-date* + "When something happened, as time ago, with the full date on hover." + {::mf/private true} + [{:keys [date]}] + (when date + (let [date (ct/inst date)] + [:time {:class (stl/css :entry-date) + :date-time (ct/format-inst date :iso) + :title (ct/format-inst date :localized-date-time)} + (ct/timeago date)]))) + +(mf/defc route* + "The branch the changes come from and main, where they land." + {::mf/private true} + [{:keys [branch-name is-review]}] + [:div {:class (stl/css :entry-route)} + [:> branch-pill* {:label (or branch-name "—") + :is-review is-review}] + [:> i/icon* {:icon-id i/arrow-right + :size "s" + :class (stl/css :entry-route-arrow)}] + [:> branch-pill* {:label (tr "workspace.branches.main") + :is-main true}]]) + +(mf/defc author* + "The profile behind an entry: avatar and name. A profile that left the + team is no longer among the team's profiles and shows as unknown." + {::mf/private true} + [{:keys [profile]}] + (if profile + [:> avatar* {:profile profile + :variant "M" + :class (stl/css :entry-avatar)}] + [:div {:class (stl/css :entry-avatar :entry-avatar-unknown)} + [:> i/icon* {:icon-id i/user :size "s"}]])) + +;; --- Merge reviews + +(mf/defc merge-entry* + {::mf/private true} + [{:keys [merge profiles]}] + (let [author (get profiles (:merged-by merge))] + [:li {:class (stl/css :entry)} + [:> author* {:profile author}] + [:div {:class (stl/css :entry-body)} + [:div {:class (stl/css :entry-head)} + [:span {:class (stl/css :entry-title)} + (or (:fullname author) (tr "workspace.branches.reviews.unknown-author"))] + [:> entry-date* {:date (:created-at merge)}]] + [:div {:class (stl/css :entry-meta)} + [:span {:class (stl/css :entry-meta-text)} + (tr "workspace.branches.reviews.merged")] + [:> route* {:branch-name (:branch-name merge)}]] + ;; the merge accepted a design review: name it, it may be gone by now + (when-let [title (:pull-request-title merge)] + [:div {:class (stl/css :entry-review)} + [:> i/icon* {:icon-id i/git-pull-request-arrow :size "s"}] + [:span {:class (stl/css :entry-review-title)} + (tr "workspace.branches.reviews.from-review" title)]]) + (if (str/blank? (:comment merge)) + [:p {:class (stl/css :entry-no-comment)} + (tr "workspace.branches.reviews.no-comment")] + [:> clamped-text* {:text (:comment merge)}])]])) + +;; --- Design reviews + +(mf/defc design-review-entry* + "A design review: the entry opens it while it is open; a finished one + (merged or closed) can no longer be opened, so the entry shows its + details instead. Its options menu shows the details of every review and + reopens a closed one for its author or a team admin, as the details + dialog does." + {::mf/private true} + [{:keys [pr profiles]}] + (let [author (get profiles (:created-by pr)) + profile (mf/deref refs/profile) + team (mf/deref refs/team) + open? (= "open" (:status pr)) + closed? (= "closed" (:status pr)) + can-reopen? (and closed? + (or (= (:created-by pr) (:id profile)) + (boolean (get-in team [:permissions :is-admin])))) + [approvals total] (prui/current-approvals pr) + + ;; each entry keeps its own menu state: the DS menu closes every + ;; other open menu by itself when one opens + menu-open* (mf/use-state false) + menu-open? (deref menu-open*) + on-toggle-menu (mf/use-fn #(swap! menu-open* not)) + on-menu-change (mf/use-fn #(reset! menu-open* %)) + + on-menu-action + (mf/use-fn + (mf/deps pr) + (fn [action] + (case action + "details" (modal/show! :pull-request-info {:pr pr}) + "reopen" (st/emit! (dwpr/reopen-pull-request (:id pr))) + nil))) + + on-open + (mf/use-fn + (mf/deps pr open?) + (fn [_] + (if open? + (st/emit! (modal/hide) + (dwpr/open-pull-request pr)) + (modal/show! :pull-request-info {:pr pr})))) + + on-key-down + (mf/use-fn + (mf/deps on-open) + (fn [event] + (when (or (kbd/enter? event) (kbd/space? event)) + (dom/prevent-default event) + (on-open event))))] + + [:li {:class (stl/css :entry :is-clickable) + :role "button" + :tab-index 0 + :title (if open? + (tr "workspace.pull-requests.actions.open-review") + (tr "workspace.pull-requests.menu.review-details")) + :on-click on-open + :on-key-down on-key-down} + [:> author* {:profile author}] + [:div {:class (stl/css :entry-body)} + [:div {:class (stl/css :entry-head)} + [:span {:class (stl/css :entry-title)} (:title pr)] + [:> pr-state-badge* {:pr pr}] + [:> entry-date* {:date (:created-at pr)}]] + [:div {:class (stl/css :entry-meta)} + [:span {:class (stl/css :entry-meta-text)} + (or (:fullname author) (tr "workspace.branches.reviews.unknown-author"))] + [:> route* {:branch-name (:branch-name pr) :is-review open?}] + (when (pos? total) + [:span {:class (stl/css :entry-approvals) + :title (tr "workspace.pull-requests.info.approvals")} + [:> i/icon* {:icon-id i/tick :size "s"}] + (dm/str approvals "/" total)])] + (when-not (str/blank? (:description pr)) + [:> clamped-text* {:text (:description pr)}])] + ;; the entry opens the review on click and on Enter: neither the + ;; menu, its trigger nor its items (portaled, but still in this + ;; React tree) may reach it + [:div {:class (stl/css :entry-aside) + :on-click dom/stop-propagation + :on-key-down dom/stop-propagation} + [:> menu* {:is-open menu-open? + :on-open-change on-menu-change + :on-action on-menu-action + :placement "bottom end" + :trigger + (mf/html + [:> icon-button* {:variant "ghost" + :icon i/menu + :aria-label (tr "labels.options") + :aria-pressed menu-open? + :on-click on-toggle-menu}])} + [:> menu-item* {:id "details"} + (tr "workspace.pull-requests.menu.review-details")] + (when can-reopen? + [:> menu-item* {:id "reopen"} + (tr "workspace.pull-requests.actions.reopen")])]]])) + +;; --- Dialog + +(mf/defc list-state* + "What a tab shows while its list is not there: loading, a failure with + a retry, or an empty list." + {::mf/private true} + [{:keys [status icon empty-text on-retry]}] + (case status + :loading + [:div {:class (stl/css :reviews-empty) :role "status"} + [:> empty-state* {:icon i/git-pull-request-arrow + :text (tr "workspace.branches.reviews.loading")}]] + + :error + [:div {:class (stl/css :reviews-empty) :role "alert"} + [:> empty-state* {:icon i/triangle-alert + :text (tr "workspace.branches.reviews.error")}] + [:> button* {:variant "secondary" + :icon i/reload + :on-click on-retry} + (tr "labels.retry")]] + + [:div {:class (stl/css :reviews-empty)} + [:> empty-state* {:icon icon :text empty-text}]])) + +(mf/defc reviews-dialog* + {::mf/register modal/components + ::mf/register-as :branch-reviews} + [] + (let [merges-state (mf/deref merges) + prs-state (mf/deref refs/pull-requests) + profiles (mf/deref refs/profiles) + prs-enabled? (dwpr/enabled?) + + tab* (mf/use-state "merges") + tab (deref tab*) + filter* (mf/use-state "") + filter-v (deref filter*) + + all-merges (:data merges-state) + all-prs (when prs-enabled? (:data prs-state)) + + ;; the search reads what each entry shows: the branch, the + ;; people and the texts + shown-merges + (mf/with-memo [all-merges filter-v profiles] + (filterv #(matches? filter-v [(:branch-name %) + (:comment %) + (:pull-request-title %) + (get-in profiles [(:merged-by %) :fullname])]) + all-merges)) + + shown-prs + (mf/with-memo [all-prs filter-v profiles] + (filterv #(matches? filter-v [(:title %) + (:description %) + (:branch-name %) + (get-in profiles [(:created-by %) :fullname])]) + all-prs)) + + tabs + (mf/with-memo [all-merges all-prs prs-enabled?] + (cond-> [{:id "merges" + :label (dm/str (tr "workspace.branches.reviews.tab.merges") + " (" (count all-merges) ")")}] + prs-enabled? + (conj {:id "design" + :label (dm/str (tr "workspace.branches.reviews.tab.design") + " (" (count all-prs) ")")}))) + + on-tab-change (mf/use-fn #(reset! tab* %)) + on-filter-change (mf/use-fn #(reset! filter* (dom/get-target-val %))) + + ;; the input's wrapper takes the full width it is given: this + ;; box sets that width, so the tabs keep room for their labels + search + (mf/html + [:div {:class (stl/css :reviews-search)} + [:> input* {:variant "dense" + :icon i/search + :placeholder (tr "workspace.branches.reviews.search") + :value filter-v + :on-change on-filter-change}]]) + + on-open-change + (mf/use-fn + (fn [open?] + (when-not open? + (st/emit! (modal/hide))))) + + on-retry-merges + (mf/use-fn #(st/emit! (dwb/fetch-branch-merges)))] + + ;; before paint, so a list from an earlier opening never shows in + ;; place of the loading state + (mf/with-layout-effect [] + (st/emit! (dwb/fetch-branch-merges)) + (when (dwpr/enabled?) + (st/emit! (dwpr/fetch-pull-requests)))) + + [:> modal* {:is-open true + :on-open-change on-open-change + :size "large"} + [:> modal-header* {:title (tr "workspace.branches.reviews.title")}] + [:> modal-content* {:class (stl/css :reviews-content)} + [:> tab-switcher* {:tabs tabs + :selected tab + :on-change on-tab-change + :scrollable-panel true + :action-button-position "end" + :action-button search + :class (stl/css :reviews-tabs)} + (if (= tab "design") + (cond + (= :loading (:status prs-state)) + [:> list-state* {:status :loading}] + + (empty? shown-prs) + [:> list-state* {:icon i/git-pull-request-arrow + :empty-text (if (seq all-prs) + (tr "workspace.branches.reviews.no-results") + (tr "workspace.branches.reviews.design.empty"))}] + + :else + [:ul {:class (stl/css :entries)} + (for [pr shown-prs] + [:> design-review-entry* {:key (dm/str (:id pr)) + :pr pr + :profiles profiles}])]) + + (cond + (contains? #{:loading :error} (:status merges-state)) + [:> list-state* {:status (:status merges-state) + :on-retry on-retry-merges}] + + (empty? shown-merges) + [:> list-state* {:icon i/git-merge + :empty-text (if (seq all-merges) + (tr "workspace.branches.reviews.no-results") + (tr "workspace.branches.reviews.merges.empty"))}] + + :else + [:ul {:class (stl/css :entries)} + (for [merge shown-merges] + [:> merge-entry* {:key (dm/str (:id merge)) + :merge merge + :profiles profiles}])]))]]])) diff --git a/frontend/src/app/main/ui/workspace/sidebar/reviews.scss b/frontend/src/app/main/ui/workspace/sidebar/reviews.scss new file mode 100644 index 0000000000..63de4b3bb7 --- /dev/null +++ b/frontend/src/app/main/ui/workspace/sidebar/reviews.scss @@ -0,0 +1,240 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. +// +// Copyright (c) KALEIDOS INC Sucursal en España SL + +@use "ds/_borders.scss" as *; +@use "ds/_sizes.scss" as *; +@use "ds/mixins.scss" as *; +@use "ds/typography.scss" as *; + +// --- Dialog: the layout of the all-branches dialog + +.reviews-content { + display: flex; + flex-direction: column; + gap: var(--sp-m); + block-size: $sz-480; + min-block-size: 0; + padding: 0; +} + +.reviews-tabs { + flex: 1; + min-block-size: 0; +} + +.reviews-search { + inline-size: $sz-192; +} + +.reviews-empty { + display: flex; + flex-direction: column; + align-items: center; + gap: var(--sp-xxxl); + margin: var(--sp-xxxl) auto; + inline-size: $sz-200; +} + +// --- Entries + +.entries { + display: flex; + flex-direction: column; + gap: var(--sp-xs); + margin: 0; + padding: var(--sp-m) 0 0; + list-style: none; +} + +.entry { + // the card of the comment or the description: a step lighter than the + // row, also while a clickable row is hovered + --entry-card-bg-color: var(--color-background-tertiary); + + display: grid; + grid-template-columns: auto 1fr auto; + align-items: start; + gap: var(--sp-m); + padding: var(--sp-m) var(--sp-s); + border-radius: $br-8; + + &.is-clickable { + cursor: pointer; + + &:hover { + --entry-card-bg-color: var(--color-background-quaternary); + + background-color: var(--color-background-tertiary); + } + + &:focus-visible { + outline: $b-1 solid var(--color-accent-primary); + } + } +} + +.entry-avatar { + flex-shrink: 0; +} + +// a profile that left the team: a neutral tile the size of the avatar +.entry-avatar-unknown { + display: flex; + align-items: center; + justify-content: center; + inline-size: $sz-32; + block-size: $sz-32; + border-radius: $br-circle; + background-color: var(--color-background-quaternary); + color: var(--color-foreground-secondary); +} + +.entry-body { + display: flex; + flex-direction: column; + gap: var(--sp-xs); + min-inline-size: 0; +} + +.entry-head { + display: flex; + align-items: center; + gap: var(--sp-s); + min-inline-size: 0; +} + +.entry-title { + @include use-typography("body-medium"); + + min-inline-size: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + color: var(--color-foreground-primary); +} + +.entry-date { + @include use-typography("body-small"); + + flex-shrink: 0; + margin-inline-start: auto; + color: var(--color-foreground-secondary); +} + +.entry-meta { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: var(--sp-s); + min-inline-size: 0; +} + +.entry-meta-text { + @include use-typography("body-small"); + + color: var(--color-foreground-secondary); +} + +.entry-route { + display: flex; + align-items: center; + gap: var(--sp-xs); + min-inline-size: 0; +} + +.entry-route-arrow { + flex-shrink: 0; + color: var(--color-foreground-secondary); +} + +.entry-review { + @include use-typography("body-small"); + + display: flex; + align-items: center; + gap: var(--sp-xs); + min-inline-size: 0; + color: var(--color-foreground-secondary); +} + +.entry-review-title { + min-inline-size: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; +} + +.entry-approvals { + @include use-typography("body-small"); + + display: inline-flex; + align-items: center; + gap: var(--sp-xxs); + color: var(--color-foreground-secondary); +} + +// the empty card of the branch info dialog's description +.entry-no-comment { + @include use-typography("body-medium"); + + margin: 0; + margin-block-start: var(--sp-xs); + padding: var(--sp-s); + border: $b-1 dashed var(--color-background-quaternary); + border-radius: $br-8; + font-style: italic; + color: var(--color-foreground-secondary); +} + +.entry-aside { + display: flex; + align-items: center; +} + +// --- A long text folded to two lines, in the card of the branch info +// dialog's description + +.clamped { + display: flex; + flex-direction: column; + align-items: flex-start; + gap: var(--sp-xs); + margin-block-start: var(--sp-xs); + padding: var(--sp-s); + border-radius: $br-8; + background-color: var(--entry-card-bg-color); +} + +.clamped-text { + @include use-typography("body-medium"); + + margin: 0; + color: var(--color-foreground-primary); + white-space: pre-line; + overflow-wrap: anywhere; +} + +.is-folded { + @include two-line-text-ellipsis; +} + +.see-more { + @include use-typography("body-small"); + + padding: 0; + border: none; + background: none; + color: var(--color-accent-primary); + cursor: pointer; + + &:hover { + text-decoration: underline; + } + + &:focus-visible { + outline: $b-1 solid var(--color-accent-primary); + } +} diff --git a/frontend/test/frontend_tests/data/workspace_branches_test.cljs b/frontend/test/frontend_tests/data/workspace_branches_test.cljs index bedb5b630b..bd5d712932 100644 --- a/frontend/test/frontend_tests/data/workspace_branches_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_branches_test.cljs @@ -223,6 +223,29 @@ (done')) done)))) +(t/deftest the-comment-travels-with-the-merge-and-its-conflicts + (t/async done + (let [calls (atom []) + conflict-id (uuid/next)] + (mock/with-mocks + {rp/cmd! (cmd-stub calls conflict-id)} + (fn [done'] + (let [branch {:id (uuid/next)} + store (make-store)] + (ptk/emit! store (dwb/merge-branch branch {:comment " Ships the header "})) + (t/is (= "Ships the header" (:comment (last-params calls :merge))) + "the comment is trimmed") + (t/testing "the conflict answer hands the comment to the modal" + (t/is (= :branch-conflicts (get-in @store [::modal/modal :type]))) + (t/is (= " Ships the header " + (get-in @store [::modal/modal :props :comment])))) + (t/testing "a blank comment sends no `:comment` at all" + (ptk/emit! store (dwb/merge-branch branch {:comment " "})) + (t/is (not (contains? (last-params calls :merge) :comment)))) + (rx/dispose! store)) + (done')) + done)))) + ;;; --- Branch switcher: the entry point of the menu and the shortcut --- (defn- make-store-with @@ -310,9 +333,10 @@ (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") + (await (async/wait-for #(some? (::modal/modal @store)) + "the shortcut opens the branches dialog, which retries")) + (t/is (= :branches-list (get-in @store [::modal/modal :type])) + "no create dialog on an unknown count") (rx/dispose! store))))) (t/deftest ^:async a-listing-after-a-failed-count-settles-it @@ -375,15 +399,15 @@ (t/is (true? (:workspace-branch-switcher @store)) "forcing it open twice keeps it open") (rx/dispose! store))) -(t/deftest ^:async show-branches-opens-the-switcher-when-there-are-branches +(t/deftest ^:async show-branches-opens-the-branches-dialog-when-there-are-branches (await (mock/with-mocks* {cf/flags (conj cf/flags :branching)} - (let [store (make-store-with main-with-branches)] + (let [store (make-store-with (assoc main-with-branches :workspace-branch-switcher true))] (ptk/emit! store (dwb/show-branches)) - (await (async/wait-for #(true? (:workspace-branch-switcher @store)) - "the switcher opens")) - (await (async/settle)) - (t/is (nil? (::modal/modal @store)) "no dialog opens") + (await (async/wait-for #(some? (::modal/modal @store)) + "the branches dialog opens")) + (t/is (= :branches-list (get-in @store [::modal/modal :type]))) + (t/is (false? (:workspace-branch-switcher @store)) "the switcher closes") (rx/dispose! store))))) (t/deftest ^:async show-branches-asks-for-the-first-branch-when-there-are-none @@ -394,7 +418,6 @@ (await (async/wait-for #(some? (::modal/modal @store)) "the create dialog opens")) (t/is (= :create-branch (get-in @store [::modal/modal :type]))) - (t/is (= "Checkout" (get-in @store [::modal/modal :props :file-name]))) (t/is (not (:workspace-branch-switcher @store))) (rx/dispose! store))))) @@ -449,3 +472,31 @@ (t/is (= [{:file-id branch-id}] @navigations) "it opens the branch that was created") (rx/dispose! store)))))) + +;;; --- Reviews dialog: the merges into main --- + +(t/deftest ^:async the-merges-load-for-the-open-file + (let [calls (atom []) + merges [{:id (uuid/next) :branch-name "header" :comment "Ships it"}]] + (await (mock/with-mocks* + {rp/cmd! (mock/stub (fn [cmd params] + (swap! calls conj [cmd params]) + (rx/of merges)))} + (let [store (make-store-with main-without-branches)] + (ptk/emit! store (dwb/fetch-branch-merges)) + (await (async/wait-for #(= :loaded (get-in @store [:workspace-branch-merges :status])) + "the merges land")) + (t/is (= merges (get-in @store [:workspace-branch-merges :data]))) + (t/is (= [[:get-file-branch-merges {:file-id file-id}]] @calls) + "the open file is asked as is: the command resolves a branch to main") + (rx/dispose! store)))))) + +(t/deftest ^:async a-failed-merges-load-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-without-branches)] + (ptk/emit! store (dwb/fetch-branch-merges)) + (await (async/wait-for #(= :error (get-in @store [:workspace-branch-merges :status])) + "the failure lands")) + (t/is (nil? (get-in @store [:workspace-branch-merges :data]))) + (rx/dispose! store))))) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 841378027e..1f3a8077bb 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -6355,6 +6355,14 @@ msgstr "Text Transform" msgid "workspace.assets.ungroup" msgstr "Ungroup" +msgid "workspace.branches.archive.message" +msgstr "" +"Archive branch \"%s\"? It becomes read-only and leaves the active list. You " +"can restore it later." + +msgid "workspace.branches.archive.title" +msgstr "Archive branch" + #: src/app/main/ui/workspace/sidebar/branches.cljs:836 msgid "workspace.branches.archived" msgstr "Archived" @@ -6909,10 +6917,6 @@ msgstr "e.g. redesign-checkout" msgid "workspace.branches.create.submit" msgstr "Create branch" -#: src/app/main/ui/workspace/sidebar/branches.cljs:276 -msgid "workspace.branches.create.subtitle" -msgstr "Derived from main · %s" - #: src/app/main/data/workspace/branches.cljs:201 msgid "workspace.branches.create.success" msgstr "Branch \"%s\" created" @@ -7171,14 +7175,13 @@ msgstr "Main" msgid "workspace.branches.main-advanced" msgstr "Main has new changes. You can update this branch" +msgid "workspace.branches.main-library-note" +msgstr "Read by library consumers" + #: src/app/main/ui/workspace/sidebar/branches.cljs:659 msgid "workspace.branches.main-subtitle" msgstr "Default branch" -#: src/app/main/ui/workspace/branch_switcher.cljs:88 -msgid "workspace.branches.main-subtitle-library" -msgstr "Default branch (read by library consumers)" - #: src/app/main/ui/workspace/sidebar/branches.cljs:779 msgid "workspace.branches.materialize.accept" msgstr "Turn it into a file" @@ -7225,15 +7228,8 @@ msgstr "Restore" msgid "workspace.branches.merge.action" msgstr "Merge into main" -#: src/app/main/ui/workspace/sidebar/branches.cljs:370, src/app/main/ui/workspace/sidebar/branches.cljs:1526 -msgid "workspace.branches.merge.archive-label" -msgstr "Archive this branch" - -#: src/app/main/ui/workspace/sidebar/branches.cljs:376 -msgid "workspace.branches.merge.archive-warning" -msgstr "" -"Archived branches keep a full copy of the file so they can be restored " -"later. Keeping many archived branches can take up a lot of storage." +msgid "workspace.branches.merge.comment-label" +msgstr "Merge comment" #: src/app/main/ui/workspace/sidebar/branches.cljs:386 msgid "workspace.branches.merge.confirm-accept" @@ -7256,9 +7252,7 @@ msgstr "This branch has conflicts that must be resolved before merging" #: src/app/main/ui/workspace/sidebar/branches.cljs:378 msgid "workspace.branches.merge.delete-info" -msgstr "" -"After merging, this branch and its file will be deleted to free up storage. " -"Archive it if you might need to restore it later." +msgstr "After merging, this branch and its file will be deleted to free up storage." #: src/app/main/data/workspace/branches.cljs:451, src/app/main/data/workspace/branches.cljs:459 msgid "workspace.branches.merge.error" @@ -7284,14 +7278,66 @@ msgstr "" "This branch changes elements that can't be merged automatically yet " "(components, pages or tokens)" -#: src/app/main/ui/workspace/branch_switcher.cljs:208, src/app/main/ui/workspace/sidebar/branches.cljs:943 -msgid "workspace.branches.merged" -msgstr "Merged" - #, unused msgid "workspace.branches.new" msgstr "New" +msgid "workspace.branches.rename.title" +msgstr "Rename branch" + +msgid "workspace.branches.restore.message" +msgstr "" +"Restore branch \"%s\"? It goes back to the active list and you can edit it " +"again." + +msgid "workspace.branches.restore.title" +msgstr "Restore branch" + +msgid "workspace.branches.reviews.design.empty" +msgstr "No design reviews yet" + +msgid "workspace.branches.reviews.error" +msgstr "Couldn't load the merges" + +msgid "workspace.branches.reviews.from-review" +msgstr "From the design review \"%s\"" + +msgid "workspace.branches.reviews.loading" +msgstr "Loading reviews…" + +msgid "workspace.branches.reviews.merged" +msgstr "Merged" + +msgid "workspace.branches.reviews.merges.empty" +msgstr "No branch has been merged into main yet" + +msgid "workspace.branches.reviews.no-comment" +msgstr "No comment" + +msgid "workspace.branches.reviews.no-results" +msgstr "No reviews match your search" + +msgid "workspace.branches.reviews.search" +msgstr "Search review…" + +msgid "workspace.branches.reviews.see-less" +msgstr "See less" + +msgid "workspace.branches.reviews.see-more" +msgstr "See more" + +msgid "workspace.branches.reviews.tab.design" +msgstr "Design reviews" + +msgid "workspace.branches.reviews.tab.merges" +msgstr "Merge reviews" + +msgid "workspace.branches.reviews.title" +msgstr "Reviews" + +msgid "workspace.branches.reviews.unknown-author" +msgstr "Unknown user" + #: src/app/main/ui/workspace/sidebar/branches.cljs:787 msgid "workspace.branches.search.placeholder" msgstr "Search branch…" @@ -7332,6 +7378,9 @@ msgstr "New branch" msgid "workspace.branches.switcher.new-from-main" msgstr "Branches are created from main" +msgid "workspace.branches.switcher.reviews" +msgstr "Reviews" + #: src/app/main/ui/workspace/branch_switcher.cljs:309 msgid "workspace.branches.switcher.tooltip" msgstr "Switch branch" @@ -9369,7 +9418,7 @@ msgstr "TRY PLUGIN" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:426, src/app/main/ui/workspace/sidebar/pull_requests.cljs:505, src/app/main/ui/workspace/sidebar/pull_requests.cljs:564 msgid "workspace.pull-requests.actions.close" -msgstr "Cancel pull request" +msgstr "Cancel design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:440, src/app/main/ui/workspace/sidebar/pull_requests.cljs:561, src/app/main/ui/workspace/sidebar/pull_requests.cljs:754 msgid "workspace.pull-requests.actions.merge" @@ -9381,7 +9430,7 @@ msgstr "Open review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:430, src/app/main/ui/workspace/sidebar/pull_requests.cljs:567 msgid "workspace.pull-requests.actions.reopen" -msgstr "Reopen pull request" +msgstr "Reopen design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:359, src/app/main/ui/workspace/sidebar/pull_requests.cljs:537, src/app/main/ui/workspace/sidebar/pull_requests.cljs:722 msgid "workspace.pull-requests.badge.conflicts" @@ -9409,7 +9458,7 @@ msgstr "Review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:710 msgid "workspace.pull-requests.banner.reviewing" -msgstr "Reviewing pull request" +msgstr "Reviewing the design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:504 msgid "workspace.pull-requests.close.message" @@ -9419,11 +9468,11 @@ msgstr "" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:503 msgid "workspace.pull-requests.close.title" -msgstr "Cancel pull request" +msgstr "Cancel design review" #: src/app/main/data/workspace/pull_requests.cljs:142, src/app/main/data/workspace/pull_requests.cljs:255 msgid "workspace.pull-requests.create.already-exists" -msgstr "This branch already has an open pull request" +msgstr "This branch already has an open design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:167 msgid "workspace.pull-requests.create.description-label" @@ -9435,7 +9484,7 @@ msgstr "What changed, and what needs review or validation?" #: src/app/main/data/workspace/pull_requests.cljs:143 msgid "workspace.pull-requests.create.error" -msgstr "An error occurred while creating the pull request" +msgstr "An error occurred while creating the design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:193 msgid "workspace.pull-requests.create.info" @@ -9462,7 +9511,7 @@ msgstr "Reviewers" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:202 msgid "workspace.pull-requests.create.submit" -msgstr "Create pull request" +msgstr "Create design review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:152 msgid "workspace.pull-requests.create.subtitle" @@ -9470,15 +9519,15 @@ msgstr "Request a review of the changes on \"%s\"" #: src/app/main/data/workspace/pull_requests.cljs:137 msgid "workspace.pull-requests.create.success" -msgstr "Pull request \"%s\" created" +msgstr "Design review \"%s\" created" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:150 msgid "workspace.pull-requests.create.title" -msgstr "Create pull request" +msgstr "Create design review" #: src/app/main/ui/workspace/left_header.cljs:163 msgid "workspace.pull-requests.header-badge-tooltip" -msgstr "Reviewing pull request: %s" +msgstr "Reviewing the design review: %s" #: src/app/main/ui/workspace/left_header.cljs:177 msgid "workspace.pull-requests.header-label" @@ -9514,19 +9563,19 @@ msgstr "Review updated" #: src/app/main/data/workspace/pull_requests.cljs:252 msgid "workspace.pull-requests.lifecycle.branch-gone" -msgstr "The pull request's branch is no longer open" +msgstr "The design review's branch is no longer open" #: src/app/main/data/workspace/pull_requests.cljs:230 msgid "workspace.pull-requests.lifecycle.closed" -msgstr "Pull request closed" +msgstr "Design review closed" #: src/app/main/data/workspace/pull_requests.cljs:160, src/app/main/data/workspace/pull_requests.cljs:187, src/app/main/data/workspace/pull_requests.cljs:213, src/app/main/data/workspace/pull_requests.cljs:237, src/app/main/data/workspace/pull_requests.cljs:257 msgid "workspace.pull-requests.lifecycle.error" -msgstr "An error occurred while updating the pull request" +msgstr "An error occurred while updating the design review" #: src/app/main/data/workspace/pull_requests.cljs:246 msgid "workspace.pull-requests.lifecycle.reopened" -msgstr "Pull request reopened" +msgstr "Design review reopened" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:555, src/app/main/ui/workspace/sidebar/pull_requests.cljs:731 msgid "workspace.pull-requests.menu.info" @@ -9550,7 +9599,7 @@ msgstr "New" #: src/app/main/data/workspace/pull_requests.cljs:396 msgid "workspace.pull-requests.preview.unavailable" -msgstr "The review sandbox is not available (the pull request may be closed)" +msgstr "The review sandbox is not available (the design review may be closed)" #: src/app/main/data/workspace/pull_requests.cljs:182 msgid "workspace.pull-requests.publish.success" @@ -9618,7 +9667,7 @@ msgstr "Submit review" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:632 msgid "workspace.pull-requests.section.empty" -msgstr "No pull requests yet. Create one from a branch to request a review." +msgstr "No design reviews yet. Create one from a branch to request a review." #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:639 msgid "workspace.pull-requests.section.history" @@ -9626,7 +9675,7 @@ msgstr "Finished" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:611 msgid "workspace.pull-requests.section.title" -msgstr "Pull requests" +msgstr "Design reviews" #: src/app/main/ui/workspace/sidebar/pull_requests.cljs:100 #, unused