From a3d5527f100c96f1d879ef509db758c2fe1c2711 Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Sat, 24 May 2025 14:34:47 +0100 Subject: [PATCH] appview: rework compare page trigger comparison on button click, this simplifes a variety of things: - we can load a diff on page visit without javascript - we can avoid modifying url using javascript and breaking back buttons - we can avoid a lot of javascript code Signed-off-by: oppiliappan --- appview/pages/funcmap.go | 8 +- appview/pages/pages.go | 20 ++- appview/pages/templates/repo/compare.html | 166 ------------------ .../pages/templates/repo/compare/compare.html | 15 ++ appview/pages/templates/repo/compare/new.html | 31 ++++ .../templates/repo/fragments/compareForm.html | 73 ++++++++ appview/pages/templates/repo/index.html | 2 +- appview/state/repo.go | 136 +++++++------- appview/state/router.go | 4 +- 9 files changed, 207 insertions(+), 248 deletions(-) delete mode 100644 appview/pages/templates/repo/compare.html create mode 100644 appview/pages/templates/repo/compare/compare.html create mode 100644 appview/pages/templates/repo/compare/new.html create mode 100644 appview/pages/templates/repo/fragments/compareForm.html diff --git a/appview/pages/funcmap.go b/appview/pages/funcmap.go index 038e7451..d535a390 100644 --- a/appview/pages/funcmap.go +++ b/appview/pages/funcmap.go @@ -133,16 +133,18 @@ func funcMap() template.FuncMap { "sequence": func(n int) []struct{} { return make([]struct{}, n) }, - "subslice": func(slice any, start, end int) any { + // take atmost N items from this slice + "take": func(slice any, n int) any { v := reflect.ValueOf(slice) if v.Kind() != reflect.Slice && v.Kind() != reflect.Array { return nil } - if start < 0 || start > v.Len() || end > v.Len() || start > end { + if v.Len() == 0 { return nil } - return v.Slice(start, end).Interface() + return v.Slice(0, min(n, v.Len()-1)).Interface() }, + "markdown": func(text string) template.HTML { rctx := &markup.RenderContext{RendererType: markup.RendererTypeDefault} return template.HTML(bluemonday.UGCPolicy().Sanitize(rctx.RenderMarkdown(text))) diff --git a/appview/pages/pages.go b/appview/pages/pages.go index a2dfef70..e231baae 100644 --- a/appview/pages/pages.go +++ b/appview/pages/pages.go @@ -871,13 +871,31 @@ type RepoCompareParams struct { Tags []*types.TagReference Base string Head string + Diff *types.NiceDiff Active string } func (p *Pages) RepoCompare(w io.Writer, params RepoCompareParams) error { params.Active = "overview" - return p.executeRepo("repo/compare", w, params) + return p.executeRepo("repo/compare/compare", w, params) +} + +type RepoCompareNewParams struct { + LoggedInUser *oauth.User + RepoInfo repoinfo.RepoInfo + Forks []db.Repo + Branches []types.Branch + Tags []*types.TagReference + Base string + Head string + + Active string +} + +func (p *Pages) RepoCompareNew(w io.Writer, params RepoCompareNewParams) error { + params.Active = "overview" + return p.executeRepo("repo/compare/new", w, params) } type RepoCompareAllowPullParams struct { diff --git a/appview/pages/templates/repo/compare.html b/appview/pages/templates/repo/compare.html deleted file mode 100644 index 6621b1a2..00000000 --- a/appview/pages/templates/repo/compare.html +++ /dev/null @@ -1,166 +0,0 @@ -{{ define "title" }} - {{ if and .Head .Base }} - comparing {{ .Base }} and - {{ .Head }} - {{ else }} - new comparison - {{ end }} -{{ end }} - -{{ define "repoContent" }} -
-

- Compare changes -

-

Choose any two refs to compare.

- -
-
-
- base: - - -
- - {{ i "arrow-left" "w-4 h-4" }} - - -
- compare: - - -
-
-
-
- - -{{ end }} - -{{ define "repoAfter" }} -
-
-{{ end }} diff --git a/appview/pages/templates/repo/compare/compare.html b/appview/pages/templates/repo/compare/compare.html new file mode 100644 index 00000000..cc261872 --- /dev/null +++ b/appview/pages/templates/repo/compare/compare.html @@ -0,0 +1,15 @@ +{{ define "title" }} + comparing {{ .Base }} and {{ .Head }} on {{ .RepoInfo.FullName }} +{{ end }} + +{{ define "repoContent" }} + {{ template "repo/fragments/compareForm" . }} + {{ $isPushAllowed := and .LoggedInUser .RepoInfo.Roles.IsPushAllowed }} + {{ if $isPushAllowed }} + {{ template "repo/fragments/compareAllowPull" . }} + {{ end }} +{{ end }} + +{{ define "repoAfter" }} + {{ template "repo/fragments/diff" (list .RepoInfo.FullName .Diff) }} +{{ end }} diff --git a/appview/pages/templates/repo/compare/new.html b/appview/pages/templates/repo/compare/new.html new file mode 100644 index 00000000..f7d77796 --- /dev/null +++ b/appview/pages/templates/repo/compare/new.html @@ -0,0 +1,31 @@ +{{ define "title" }} + compare refs on {{ .RepoInfo.FullName }} +{{ end }} + +{{ define "repoContent" }} + {{ template "repo/fragments/compareForm" . }} +{{ end }} + +{{ define "repoAfter" }} +
+
+

+ Recently updated branches in this repository: +

+ {{ block "recentBranchList" $ }} {{ end }} +
+
+{{ end }} + +{{ define "recentBranchList" }} +
+ {{ range $br := take .Branches 5 }} + +
+ {{ $br.Name }} + +
+ + {{ end }} +
+{{ end }} diff --git a/appview/pages/templates/repo/fragments/compareForm.html b/appview/pages/templates/repo/fragments/compareForm.html new file mode 100644 index 00000000..32c26dbd --- /dev/null +++ b/appview/pages/templates/repo/fragments/compareForm.html @@ -0,0 +1,73 @@ +{{ define "repo/fragments/compareForm" }} +
+

+ Compare changes +

+

Choose any two refs to compare.

+ +
+
+ + {{ block "dropdown" (list $ "base" $.Base) }} {{ end }} +
+ + {{ i "arrow-left" "w-4 h-4" }} + +
+ + {{ block "dropdown" (list $ "head" $.Head) }} {{ end }} +
+ +
+
+ +{{ end }} + +{{ define "dropdown" }} +{{ $root := index . 0 }} +{{ $name := index . 1 }} +{{ $default := index . 2 }} + +{{ end }} diff --git a/appview/pages/templates/repo/index.html b/appview/pages/templates/repo/index.html index 02e4cb6f..3bc5d704 100644 --- a/appview/pages/templates/repo/index.html +++ b/appview/pages/templates/repo/index.html @@ -101,7 +101,7 @@ {{ end }} diff --git a/appview/state/repo.go b/appview/state/repo.go index fa77b512..e57fb010 100644 --- a/appview/state/repo.go +++ b/appview/state/repo.go @@ -12,6 +12,7 @@ import ( "net/http" "path" "slices" + "sort" "strconv" "strings" "time" @@ -2056,7 +2057,7 @@ func (s *State) ForkRepo(w http.ResponseWriter, r *http.Request) { } } -func (s *State) RepoCompare(w http.ResponseWriter, r *http.Request) { +func (s *State) RepoCompareNew(w http.ResponseWriter, r *http.Request) { user := s.oauth.GetUser(r) f, err := s.fullyResolvedRepo(r) if err != nil { @@ -2064,20 +2065,6 @@ func (s *State) RepoCompare(w http.ResponseWriter, r *http.Request) { return } - // if user is navigating to one of - // /compare/{base}/{head} - // /compare/{base}...{head} - base := chi.URLParam(r, "base") - head := chi.URLParam(r, "head") - if base == "" && head == "" { - rest := chi.URLParam(r, "*") // master...feature/xyz - parts := strings.SplitN(rest, "...", 2) - if len(parts) == 2 { - base = parts[0] - head = parts[1] - } - } - us, err := knotclient.NewUnsignedClient(f.Knot, s.config.Core.Dev) if err != nil { log.Printf("failed to create unsigned client for %s", f.Knot) @@ -2085,12 +2072,36 @@ func (s *State) RepoCompare(w http.ResponseWriter, r *http.Request) { return } - branches, err := us.Branches(f.OwnerDid(), f.RepoName) + result, err := us.Branches(f.OwnerDid(), f.RepoName) if err != nil { s.pages.Notice(w, "compare-error", "Failed to produce comparison. Try again later.") log.Println("failed to reach knotserver", err) return } + branches := result.Branches + sort.Slice(branches, func(i int, j int) bool { + return branches[i].Commit.Committer.When.After(branches[j].Commit.Committer.When) + }) + + var defaultBranch string + for _, b := range branches { + if b.IsDefault { + defaultBranch = b.Name + } + } + + base := defaultBranch + head := defaultBranch + + params := r.URL.Query() + queryBase := params.Get("base") + queryHead := params.Get("head") + if queryBase != "" { + base = queryBase + } + if queryHead != "" { + head = queryHead + } tags, err := us.Tags(f.OwnerDid(), f.RepoName) if err != nil { @@ -2099,32 +2110,19 @@ func (s *State) RepoCompare(w http.ResponseWriter, r *http.Request) { return } - var forks []db.Repo - if user != nil { - var err error - forks, err = db.GetForksByDid(s.db, user.Did) - if err != nil { - s.pages.Notice(w, "compare-error", "Failed to produce comparison. Try again later.") - log.Println("failed to get forks", err) - return - } - } - repoinfo := f.RepoInfo(s, user) - s.pages.RepoCompare(w, pages.RepoCompareParams{ + s.pages.RepoCompareNew(w, pages.RepoCompareNewParams{ LoggedInUser: user, RepoInfo: repoinfo, - Forks: forks, - Branches: branches.Branches, + Branches: branches, Tags: tags.Tags, Base: base, Head: head, }) - } -func (s *State) RepoCompareAllowPullFragment(w http.ResponseWriter, r *http.Request) { +func (s *State) RepoCompare(w http.ResponseWriter, r *http.Request) { user := s.oauth.GetUser(r) f, err := s.fullyResolvedRepo(r) if err != nil { @@ -2132,76 +2130,66 @@ func (s *State) RepoCompareAllowPullFragment(w http.ResponseWriter, r *http.Requ return } - s.pages.RepoCompareAllowPullFragment(w, pages.RepoCompareAllowPullParams{ - Head: chi.URLParam(r, "head"), - Base: chi.URLParam(r, "base"), - RepoInfo: f.RepoInfo(s, user), - LoggedInUser: user, - }) -} - -func (s *State) RepoCompareDiffFragment(w http.ResponseWriter, r *http.Request) { - f, err := s.fullyResolvedRepo(r) - if err != nil { - log.Println("failed to get repo and knot", err) - return - } - user := s.oauth.GetUser(r) - + // if user is navigating to one of + // /compare/{base}/{head} + // /compare/{base}...{head} base := chi.URLParam(r, "base") head := chi.URLParam(r, "head") + if base == "" && head == "" { + rest := chi.URLParam(r, "*") // master...feature/xyz + parts := strings.SplitN(rest, "...", 2) + if len(parts) == 2 { + base = parts[0] + head = parts[1] + } + } if base == "" || head == "" { - s.pages.Notice(w, "compare-error", "Invalid ref format.") + log.Printf("invalid comparison") + s.pages.Error404(w) return } us, err := knotclient.NewUnsignedClient(f.Knot, s.config.Core.Dev) + if err != nil { + log.Printf("failed to create unsigned client for %s", f.Knot) + s.pages.Error503(w) + return + } + + branches, err := us.Branches(f.OwnerDid(), f.RepoName) if err != nil { s.pages.Notice(w, "compare-error", "Failed to produce comparison. Try again later.") log.Println("failed to reach knotserver", err) return } - formatPatch, err := us.Compare(f.OwnerDid(), f.RepoName, base, head) + tags, err := us.Tags(f.OwnerDid(), f.RepoName) if err != nil { s.pages.Notice(w, "compare-error", "Failed to produce comparison. Try again later.") - log.Println("failed to compare", err) + log.Println("failed to reach knotserver", err) return } - diff := patchutil.AsNiceDiff(formatPatch.Patch, base) - branches, err := us.Branches(f.OwnerDid(), f.RepoName) + formatPatch, err := us.Compare(f.OwnerDid(), f.RepoName, base, head) if err != nil { s.pages.Notice(w, "compare-error", "Failed to produce comparison. Try again later.") - log.Println("failed to fetch branches", err) + log.Println("failed to compare", err) return } + diff := patchutil.AsNiceDiff(formatPatch.Patch, base) + log.Println(formatPatch) repoinfo := f.RepoInfo(s, user) - w.Header().Add("Hx-Push-Url", fmt.Sprintf("/%s/compare/%s...%s", f.OwnerSlashRepo(), base, head)) - w.Header().Add("Content-Type", "text/html") - s.pages.RepoCompareDiff(w, pages.RepoCompareDiffParams{ + s.pages.RepoCompare(w, pages.RepoCompareParams{ LoggedInUser: user, RepoInfo: repoinfo, - Diff: diff, + Branches: branches.Branches, + Tags: tags.Tags, + Base: base, + Head: head, + Diff: &diff, }) - // checks if pull is allowed and performs an htmx oob-swap - // by writing to the same http.ResponseWriter - if user != nil { - if slices.ContainsFunc(branches.Branches, func(branch types.Branch) bool { - return branch.Name == head || branch.Name == base - }) { - if repoinfo.Roles.IsPushAllowed() { - s.pages.RepoCompareAllowPullFragment(w, pages.RepoCompareAllowPullParams{ - LoggedInUser: user, - RepoInfo: repoinfo, - Base: base, - Head: head, - }) - } - } - } } diff --git a/appview/state/router.go b/appview/state/router.go index 4f7ad712..7f076431 100644 --- a/appview/state/router.go +++ b/appview/state/router.go @@ -119,7 +119,7 @@ func (s *State) UserRouter() http.Handler { }) r.Route("/compare", func(r chi.Router) { - r.Get("/", s.RepoCompare) + r.Get("/", s.RepoCompareNew) // start an new comparison // we have to wildcard here since we want to support GitHub's compare syntax // /compare/{ref1}...{ref2} @@ -127,8 +127,6 @@ func (s *State) UserRouter() http.Handler { // /compare/master...some/feature // /compare/master...example.com:another/feature <- this is a fork r.Get("/{base}/{head}", s.RepoCompare) - r.Get("/diff/{base}/{head}", s.RepoCompareDiffFragment) - r.Get("/allow-pull/{base}/{head}", s.RepoCompareAllowPullFragment) r.Get("/*", s.RepoCompare) }) -- 2.51.2