Skip to content

fix(front): pin default branch on project page - #1238

Open
shiroyasha wants to merge 1 commit into
semaphoreio:mainfrom
superplanehq:pin-default-branch-project-page
Open

shiroyasha wants to merge 1 commit into
semaphoreio:mainfrom
superplanehq:pin-default-branch-project-page

Conversation

@shiroyasha

Copy link
Copy Markdown

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_branch at 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.

[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 skipi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_namepreload_requesterpreload_commit_datapreload_pipelinespreload_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_error non-nil guard at model.ex:349. The pre-existing error tests don't combine a fetch error with pinning, so that clause is never executed.
  • find_latest/1 raising. Both pin-path tests mock it to return a model or nil, so the failure mode in P1 is untested.
  • Anything above the model layer. The stale-row test swaps find_latest/1 for Workflow.find/1 via with_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/3 evaluates first_page?(params) twice in the same cond (model.ex:378, :381).
  • prepend_stale_default_branch/3 takes a bare project_id while its sibling helpers take the whole params struct — inconsistent for no benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants