fix(native): mirror the loaded Pulls screens in their skeletons, and fix the Description heading in place

CHANGELOG.md

@@ -22,6 +22,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - A session's browser is no longer closed without asking. When no pane shows it and nobody has used it for five minutes, a toast asks whether to close it: Close closes it, and Keep open, or swiping the toast away, keeps it for another five minutes. Unanswered, the toast stays up and the browser stays open, so a page left open over lunch is still there when you come back. Using the browser, or the agent using it, takes the toast down. Before, the browser was closed after five minutes away from its session, and its page, tabs and login were lost.
 - The sidebar's resize strip sits on the sidebar's own right edge, as the Code and Review panel's does, instead of in the gap between the sidebar and the sessions.
 
+- On the Pulls screen's Overview, the Description heading and its ⋯ stay in place at the top of the column while the description scrolls under them, over a separator, and the ⋯ sits at the column's right edge.
+- The Pulls screen's loading placeholders match what loads in their place: the index shows rows the height of real ones with the checks column where it lands, filling the screen, and a pull request shows its title row with its buttons, the real tab strip, and the open tab's layout (the description beside its facts, commits, checks, the conversation's avatars and comments, or the files' toolbar, tree and cards) instead of a few generic bars. The conversation and the diff show the same placeholders while they load, in place of a line of text.
+
 ### Fixed
 
 - Code quoted in a review thread on the Pulls screen shows its indentation instead of a box for every tab.

native/DESIGN.md

@@ -162,8 +162,22 @@ the corner at `settingsCloseInset` (14px). Every other screen body follows the s
 
 ### Loading
 
-A skeleton mirrors the screen it stands for: the same insets, line heights and columns, a bar where each
-piece of text will land, so nothing moves when the content arrives. A generic block of bars is a bug.
+A skeleton is the loaded screen with a bar where each piece of text will land, faithful to it in every
+measure, so nothing moves when the content arrives:
+
+- the same insets, columns and separators, laid out by the same constants the loaded screen uses;
+- a row the loaded row's exact height, its bars centred in the text's line height (`TextSM` in 20px, `TextXS`
+  in 16px) and as tall as the font size, never thinner;
+- a list as many rows as fill the area, not a handful stacked at the top;
+- a fixed column (a row's checks, a facts column) keeps its width and its bar where the value sits.
+
+A test pins the skeleton row's height to the loaded row's. A generic block of bars is a bug, and so is a
+skeleton that only looks close: compare it with a shot of the loaded screen at the same size.
+
+### Fixed headings
+
+A heading that stays put while the content under it scrolls (the Overview's Description) sits on a
+`th.Separator` across its column, so the edge the content scrolls under reads as one.
 
 ### Boxes inside a screen
 

native/cmd/lich-native/pulls_files.go

@@ -141,7 +141,7 @@ func (v *pullView) filesTab(gtx C, th *ui.Theme, p *pullsScreen, d backend.PRDet
 	read := p.store.diff(p.project.Path, d.Number)
 	switch {
 	case !read.loaded:
-		return panelNotice(gtx, th, i18n.T("pulls.pullsFiles.loading"))
+		return v.skeletonFiles(gtx, th)
 	case read.err != "":
 		return panelNotice(gtx, th, i18n.T("pulls.pullsFiles.loadFailed", i18n.Args{"error": read.err}))
 	}

native/cmd/lich-native/pulls_list.go

@@ -222,7 +222,7 @@ func (p *pullsScreen) row(gtx C, th *ui.Theme, pr backend.PRSummary, now time.Ti
 }
 
 func rowContent(gtx C, th *ui.Theme, pr backend.PRSummary, checkout string, now time.Time) D {
-	return layout.Inset{Left: pullsRowPadX, Right: pullsRowPadX, Top: ui.Space(2.5), Bottom: ui.Space(2.5)}.Layout(gtx, func(gtx C) D {
+	return layout.Inset{Left: pullsRowPadX, Right: pullsRowPadX, Top: pullsRowPadY, Bottom: pullsRowPadY}.Layout(gtx, func(gtx C) D {
 		return layout.Flex{Alignment: layout.Middle}.Layout(gtx,
 			layout.Flexed(1, func(gtx C) D {
 				return layout.Flex{Axis: layout.Vertical}.Layout(gtx,
@@ -396,20 +396,49 @@ func metaGlyph(gtx C, th *ui.Theme, ic *icons.Icon, text string, fg color.NRGBA)
 	)
 }
 
-// listSkeleton is ListSkeleton: ragged title bars so it reads as a list.
+// listSkeletonMaxRows bounds the skeleton under an unbounded height; no
+// island is that tall.
+const listSkeletonMaxRows = 60
+
+// listSkeleton is the index while it loads: as many rows as the island
+// holds, each skeletonRow over a separator, as row draws them.
 func listSkeleton(gtx C, th *ui.Theme) D {
-	widths := []float32{0.92, 0.8, 1, 0.75, 0.83}
-	return layout.Inset{Left: ui.Space(3.5), Right: ui.Space(3.5), Top: ui.Space(2)}.Layout(gtx, func(gtx C) D {
-		full := gtx.Metric.PxToDp(gtx.Constraints.Max.X)
-		var rows []layout.FlexChild
-		for _, w := range widths {
-			rows = append(rows,
-				layout.Rigid(th.Skeleton(full*unit.Dp(w), ui.Space(3)).Layout),
-				ui.VGap(1.5),
-				layout.Rigid(th.Skeleton(full/3, ui.Space(2.5)).Layout),
-				ui.VGap(3),
-			)
-		}
-		return layout.Flex{Axis: layout.Vertical}.Layout(gtx, rows...)
+	titles := []float32{0.62, 0.48, 0.7, 0.55, 0.4, 0.66, 0.5}
+	rowHeight := gtx.Dp(2*pullsRowPadY+skeletonSMLine+ui.Space(1)+skeletonXSLine) + gtx.Dp(1)
+	count := min(gtx.Constraints.Max.Y/rowHeight+1, listSkeletonMaxRows)
+	var rows []layout.FlexChild
+	for i := range count {
+		title := titles[i%len(titles)]
+		rows = append(rows, layout.Rigid(func(gtx C) D { return skeletonRow(gtx, th, title) }), layout.Rigid(th.Separator))
+	}
+	return layout.Flex{Axis: layout.Vertical}.Layout(gtx, rows...)
+}
+
+// skeletonRow is rowContent with a bar where its text lands: the number and
+// title, the meta line under them, and the checks chip in its column.
+func skeletonRow(gtx C, th *ui.Theme, title float32) D {
+	gtx.Constraints.Min.X = gtx.Constraints.Max.X
+	return layout.Inset{Left: pullsRowEdge + pullsRowPadX, Right: pullsRowEdge + pullsRowPadX, Top: pullsRowPadY, Bottom: pullsRowPadY}.Layout(gtx, func(gtx C) D {
+		return layout.Flex{Alignment: layout.Middle}.Layout(gtx,
+			layout.Flexed(1, func(gtx C) D {
+				full := gtx.Metric.PxToDp(gtx.Constraints.Max.X)
+				return layout.Flex{Axis: layout.Vertical}.Layout(gtx,
+					layout.Rigid(func(gtx C) D {
+						return layout.Flex{}.Layout(gtx,
+							layout.Rigid(skeletonBar(th, ui.Space(10), skeletonSMLine, ui.Space(3.5))),
+							ui.Gap(1.5),
+							layout.Rigid(skeletonBar(th, full*unit.Dp(title), skeletonSMLine, ui.Space(3.5))),
+						)
+					}),
+					ui.VGap(1),
+					layout.Rigid(skeletonBar(th, full*unit.Dp(title)*0.6, skeletonXSLine, ui.Space(3))),
+				)
+			}),
+			ui.Gap(4),
+			layout.Rigid(func(gtx C) D {
+				return cell(gtx, rowChecksWidth, skeletonBar(th, ui.Space(9), skeletonXSLine, ui.Space(3)))
+			}),
+			layout.Rigid(layout.Spacer{Width: rowVerdictWidth + rowCheckoutWidth}.Layout),
+		)
 	})
 }

native/cmd/lich-native/pulls_list_test.go

@@ -157,3 +157,16 @@ func TestStepAlongHoldsAtTheEnds(t *testing.T) {
 		t.Errorf("no rows stepped to %d", got)
 	}
 }
+
+// The skeleton's row takes the loaded row's room, so nothing moves when the
+// index arrives.
+func TestListSkeletonRowIsALoadedRowsHeight(t *testing.T) {
+	th := rowMetaTheme(t)
+	gtx := layout.Context{Ops: new(op.Ops), Metric: unit.Metric{PxPerDp: 1, PxPerSp: 1}, Constraints: layout.Constraints{Max: image.Pt(1200, 800)}}
+	pr := backend.PRSummary{Number: 1003, Title: "feat: a title", Author: "ana", HeadRefName: "feat/x",
+		UpdatedAt: rowMetaNow.Format(time.RFC3339), Checks: backend.ChecksRollup{Total: 7}}
+	loaded := rowContent(gtx, th, pr, "fuzzy-pine", rowMetaNow).Size.Y
+	if got := skeletonRow(gtx, th, 0.5).Size.Y; got != loaded {
+		t.Errorf("skeleton row is %dpx tall, a loaded row %dpx", got, loaded)
+	}
+}

native/cmd/lich-native/pulls_panels.go

@@ -237,6 +237,9 @@ func (v *pullView) panelLayout(gtx C, th *ui.Theme, p *pullsScreen, d backend.PR
 	case tabFiles:
 		return v.filesTab(gtx, th, p, d, act)
 	case tabConversation:
+		if read := p.store.conversation(p.project.Path, d.Number); read.val == nil && read.err == "" {
+			return skeletonConversation(gtx, th)
+		}
 		rows = v.conversationRows(gtx, th, p, d, act)
 	case tabCommits:
 		rows = v.commitRows(gtx, th, d)
@@ -264,19 +267,38 @@ var (
 	overviewProseWidth = unit.Dp(720)
 	overviewFactsWidth = unit.Dp(320)
 	overviewSplitWidth = unit.Dp(760)
+	// overviewHeadingBottom is the room under the fixed Description heading,
+	// over its separator.
+	overviewHeadingBottom = ui.Space(2.5)
 )
 
 // overview is the description beside the pull request's facts.
 func (v *pullView) overview(gtx C, th *ui.Theme, p *pullsScreen, d backend.PRDetail, act *pullsAction) D {
-	rows := v.overviewRows(th, p, d, act)
+	rows := v.overviewRows(th, d, act)
+	heading := func(gtx C) D {
+		return layout.Inset{Left: pullsPadX, Right: pullsPadX, Top: ui.Space(4)}.Layout(gtx, func(gtx C) D {
+			return v.descriptionHeading(gtx, th, p, d)
+		})
+	}
 	if gtx.Constraints.Max.X < gtx.Dp(overviewSplitWidth) {
 		facts := func(gtx C) D {
 			return layout.Inset{Left: pullsPadX, Right: pullsPadX, Top: ui.Space(4)}.Layout(gtx, func(gtx C) D { return v.facts(gtx, th, p, d) })
 		}
-		return v.panelList(gtx, th, append([]layout.Widget{facts}, rows...))
+		return v.panelList(gtx, th, append([]layout.Widget{facts, heading}, rows...))
+	}
+	// The heading stays put over the description scrolling under it, the
+	// separator marking where the scroll starts.
+	description := func(gtx C) D {
+		return layout.Flex{Axis: layout.Vertical}.Layout(gtx,
+			layout.Rigid(func(gtx C) D {
+				return layout.Inset{Top: ui.Space(1), Bottom: overviewHeadingBottom}.Layout(gtx, heading)
+			}),
+			layout.Rigid(th.Separator),
+			layout.Flexed(1, func(gtx C) D { return v.panelList(gtx, th, rows) }),
+		)
 	}
 	return layout.Flex{}.Layout(gtx,
-		layout.Flexed(1, func(gtx C) D { return v.panelList(gtx, th, rows) }),
+		layout.Flexed(1, description),
 		layout.Rigid(func(gtx C) D { return verticalRule(gtx, th) }),
 		layout.Rigid(func(gtx C) D {
 			w := gtx.Dp(overviewFactsWidth)
@@ -328,28 +350,22 @@ func (v *pullView) checkoutFact(th *ui.Theme, p *pullsScreen, d backend.PRDetail
 	return out
 }
 
-// overviewRows is the description under its heading, held to a reading
-// width.
-func (v *pullView) overviewRows(th *ui.Theme, p *pullsScreen, d backend.PRDetail, act *pullsAction) []layout.Widget {
-	pad := func(top float32, w layout.Widget) layout.Widget {
-		return func(gtx C) D {
-			gtx.Constraints.Max.X = min(gtx.Constraints.Max.X, gtx.Dp(overviewProseWidth+2*pullsPadX))
-			return layout.Inset{Left: pullsPadX, Right: pullsPadX, Top: ui.Space(top)}.Layout(gtx, w)
-		}
-	}
+// overviewRows is the description, held to a reading width; its heading
+// spans the column.
+func (v *pullView) overviewRows(th *ui.Theme, d backend.PRDetail, act *pullsAction) []layout.Widget {
 	body := strings.TrimSpace(d.Body)
-	return []layout.Widget{
-		pad(4, func(gtx C) D { return v.descriptionHeading(gtx, th, p, d) }),
-		pad(2, func(gtx C) D {
+	return []layout.Widget{func(gtx C) D {
+		gtx.Constraints.Max.X = min(gtx.Constraints.Max.X, gtx.Dp(overviewProseWidth+2*pullsPadX))
+		return layout.Inset{Left: pullsPadX, Right: pullsPadX, Top: ui.Space(2)}.Layout(gtx, func(gtx C) D {
 			if v.edit.bodyURL == d.URL {
 				return v.descriptionField(gtx, th, d, act)
 			}
 			if body == "" {
 				return th.Text(ui.TextSM, i18n.T("pulls.pullsOverview.noDescription")).Muted().Layout(gtx)
 			}
 			return v.md.layout(gtx, th, "body:"+d.URL, body, act.open)
-		}),
-	}
+		})
+	}}
 }
 
 func sectionHeading(th *ui.Theme, text string) layout.Widget {