fix(front): pin default branch on project page - #1238
shiroyasha wants to merge 1 commit into
Conversation
[ui] Keep repo default branch first on latest-per-branch pages [test] Cover pin, extra fetch, later-page filter, and all_pipelines Signed-off-by: SuperPlane Agent <superplaneagent@superplane.com> Co-authored-by: Igor Šarčević <igor@superplane.com>
skipi
left a comment
There was a problem hiding this comment.
Approach looks right — all three surfaces named in the description do funnel through this model, and the view renders @workflows in list order without re-sorting, so pinning at the model layer is the correct seam. Two blocking issues though, both in the interaction between the new code and the code immediately around it.
P1 — the pin fetch runs outside Async.run, so a plumber error 500s the page
prepend_stale_default_branch/3 calls Front.Models.Workflow.find_latest/1, which does an unguarded match:
# front/lib/front/models/workflow.ex:53
{:ok, response} = Clients.Workflow.list(request)Clients.Workflow.list/1 returns {:error, _} on a gRPC failure or deadline (front/lib/front/clients/workflow.ex:75-78), so that raises a MatchError. Same shape one level deeper: Front.Models.Pipeline.list/2 does {:ok, response} = Clients.Pipeline.list(request) (front/lib/front/models/pipeline.ex:375), and it's on this path too via the preload chain.
Why that matters here specifically: every other remote call in load_from_api/1 runs inside Async.run/1, which uses Task.Supervisor.async_nolink + Task.yield (front/lib/front/async.ex:59, :85). A crash in there comes back as {:exit, reason} and lands in workflow_data/2 → the "We couldn't load workflows right now" banner. That funnel was added by #985, the immediately preceding commit to this file. But pin_default_branch_workflows/4 is invoked at model.ex:165, after all three Async.await calls, on the caller process — so it bypasses that protection entirely. Controllers match {:ok, model, source} = Model.get(...) (project_controller.ex:115, :826, :898), so the exception becomes a 500 rather than a degraded page.
Triggers when: main list succeeds, default branch isn't in the top 10, and plumber hiccups on the follow-up. In other words the page gets less resilient than it was before this PR, on the exact failure mode #985 was written to absorb.
Fix: give the fetch the same failure semantics as everything else around it — run it in an Async.run/await pair, or try/rescue around prepend_stale_default_branch/3 and fall back to workflows unchanged. The "something failed, don't pin" guard clause already exists at model.ex:349.
P1 — the "Newer" button makes the default branch disappear
first_page?/1 is params.page_token == "" (model.ex:204), but "newest page" and "empty token" aren't the same thing.
The Newer button carries the previous-page token: data-token="<%= @pagination.previous %>" data-direction="previous" (dashboard/partials/_pagination.html.eex:12). TokenPagination forwards both verbatim as {page_token: token, direction: direction} (assets/js/pollman_list/token_pagination.js:17), and workflow_list.js merges them into the request without ever clearing the token.
So paging Older → Newer lands back on the newest 10 rows with a non-empty page_token, which falls into reorder_default_branch/3's true -> rest branch (model.ex:385) and strips the default branch row out. The user sees 9 rows with the default branch gone — and nothing re-adds it; only "Back to Newest" (data-token="") recovers. Before this PR that row rendered in its natural position, so it's a regression on the very row the PR is trying to promote.
Fix: the prepend condition and the strip condition need to agree on what "the newest page" means. Either strip only when direction == "next" with a token present, or drop the prepend/strip pair and reorder only within the page you're on — you lose stale-branch promotion but gain an invariant that can't double- or zero-count. Worth a direction: "previous" test either way.
P2 — serial, unbatched fan-out that isn't attributable in metrics
find_latest/1 calls construct/2 with preload: true, which chains preload_project_name → preload_requester → preload_commit_data → preload_pipelines → preload_summary, and Pipeline.construct/1 adds preload_trigger_users/1 on top (front/lib/front/models/pipeline.ex:484-488). That's ~7 sequential round trips to service one extra row, versus one batched round per concern for the other ten.
The underlying clients do have their own benchmarks, but there's no call-site benchmark in this module — unlike every other step in load_from_api/1 (project_page_model_list_latest_workflows, project_page_model_decorate_workflows, …). So the pin's cost lands in shared counters like workflow.list.duration alongside unrelated callers, and otherwise just widens project_page_model_load_from_api with nothing to attribute it to.
Mitigated by the first-page cache in Model.get/2 — the 2s pollman refresh on the private page does hit cache. Still paid on: the public project page, which calls load_from_api/1 uncached on every render (project_controller.ex:898); every cache miss and refresh/1; and any non-first page.
Fix: at minimum wrap it in a Watchman.benchmark so it's visible. Better, chain it off fetch_project in an Async.run — which also resolves the P1 above.
Test gaps
Green CI and the seven new cases cover the main matrix, but not:
direction: "previous"— the P1 above. The only later-page test uses"next".- The
workflow_fetch_errornon-nil guard atmodel.ex:349. The pre-existing error tests don't combine a fetch error with pinning, so that clause is never executed. find_latest/1raising. Both pin-path tests mock it to return a model ornil, so the failure mode in P1 is untested.- Anything above the model layer. The stale-row test swaps
find_latest/1forWorkflow.find/1viawith_mock, so neither the real return shape nor the rendering of the prepended row is exercised.
The ordering assertions also lean on stub insertion order, which will be brittle to unrelated stub changes.
Nits
reorder_default_branch/3evaluatesfirst_page?(params)twice in the samecond(model.ex:378,:381).prepend_stale_default_branch/3takes a bareproject_idwhile its sibling helpers take the wholeparamsstruct — inconsistent for no benefit.
Testing SuperPlane factory.
NEWWO-1
Show the repository default branch first on the project page latest-per-branch list. Keep that branch on the first page even when it is older than other branches.
UI
The project page now pins
project.repo_default_branchat the top of the latest-per-branch workflow list.This pin applies to
/projects/:name_or_id,/projects/:name_or_id/branches, and the workflows poll partial.On the first page, the default branch workflow is first when that branch has a workflow. If the plumber page does not include it, the page fetches the latest default-branch workflow and prepends it.
Later pages omit the default branch so the extra first-page row is not repeated.
The pin does not apply when the default branch is blank, when that branch has no workflow, or when the tab is pull-request or tag only.
All-pipelines order does not change.
Created via SuperPlane.