Merge pull request #1832 from entireio/fix/trail-update-preserves-body

fix(trail): preserve body on interactive update

Author: gtrrz-victor

cmd/entire/cli/trail_cmd.go

@@ -1099,6 +1099,30 @@ func newTrailCreateRequest(title, body, branch, base, statusStr, typeStr, priori
 	return req
 }
 
+// resolveTrailUpdateBody returns the body text to seed the interactive update
+// form with. The list resource omits the description (it lives in
+// body_document, served only by the detail endpoint), so update must fetch the
+// detail body — otherwise the form prefills from the empty list body and a
+// user edit against that blank baseline can overwrite a description they never
+// saw. Best-effort: on a failed detail fetch it returns the list body plus the
+// error so the caller can warn (mirroring runTrailShow); an empty detail body
+// (older/partial server) falls back to the list body with no error.
+func resolveTrailUpdateBody(ctx context.Context, client *api.Client, forge, owner, repo string, found *api.TrailResource) (string, error) {
+	body := found.Body
+	if found.Number > 0 {
+		bt, err := fetchTrailDescription(ctx, client, forge, owner, repo, found.Number)
+		if err != nil {
+			return body, err
+		}
+		// fetchTrailDescription already trims; a non-empty result supersedes
+		// the list body, an empty one (older/partial server) leaves it intact.
+		if bt != "" {
+			body = bt
+		}
+	}
+	return body, nil
+}
+
 func newTrailUpdateCmd() *cobra.Command {
 	var statusStr, title, body, branch, typeStr, priorityStr string
 	var labelAdd, labelRemove, assigneeAdd, assigneeRemove, reviewerAdd, reviewerRemove []string
@@ -1221,7 +1245,16 @@ func runTrailUpdate(ctx context.Context, w, errW io.Writer, insecureHTTP bool, i
 			}
 			statusStr = string(metadata.Status)
 			title = metadata.Title
-			body = metadata.Body
+			// The list resource omits the description; fetch the detail body so
+			// the form prefills with the current text and change detection below
+			// compares against the real server value. Warn on a fetch failure so
+			// a blank baseline doesn't silently overwrite an unseen description.
+			seedBody, bodyErr := resolveTrailUpdateBody(ctx, client, forge, owner, repoName, found)
+			if bodyErr != nil {
+				fmt.Fprintf(errW, "Warning: could not load current trail body: %v\n", bodyErr)
+			}
+			body = seedBody
+			origStatus, origTitle, origBody := statusStr, title, body
 
 			form := NewAccessibleForm(
 				huh.NewGroup(
@@ -1240,9 +1273,12 @@ func runTrailUpdate(ctx context.Context, w, errW io.Writer, insecureHTTP bool, i
 			if formErr := form.Run(); formErr != nil {
 				return handleFormCancellation(w, "Trail update", formErr)
 			}
-			inputs.StatusChanged = true
-			inputs.TitleChanged = true
-			inputs.BodyChanged = true
+			// Only mark a field changed when the user actually edited it, so an
+			// untouched body/title/status isn't needlessly PATCHed (a no-op body
+			// PATCH would otherwise rewrite the description on every update).
+			inputs.StatusChanged = statusStr != origStatus
+			inputs.TitleChanged = title != origTitle
+			inputs.BodyChanged = body != origBody
 		}
 
 		statusStr = strings.TrimSpace(statusStr)

cmd/entire/cli/trail_cmd_test.go

@@ -591,6 +591,70 @@ func TestFetchTrailDescription_ReadsNestedBodyDocument(t *testing.T) {
 	}
 }
 
+func TestResolveTrailUpdateBody_PrefersDetailSnapshot(t *testing.T) {
+	t.Parallel()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		if _, err := io.WriteString(w, `{"trail":{"number":42,"body_document":{"text_snapshot":"the real body"}},"checkpoints":[],"has_write_permission":true}`); err != nil {
+			t.Errorf("write response: %v", err)
+		}
+	}))
+	defer srv.Close()
+
+	client := api.NewClientWithBaseURL("tok", srv.URL)
+	// The list resource omits the description, so found.Body is empty. The
+	// seed must come from the detail endpoint, not the empty list body.
+	found := &api.TrailResource{Number: 42, Body: ""}
+	body, err := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
+	if err != nil {
+		t.Fatalf("resolveTrailUpdateBody: %v", err)
+	}
+	if body != "the real body" {
+		t.Fatalf("body = %q, want %q", body, "the real body")
+	}
+}
+
+func TestResolveTrailUpdateBody_FallsBackToListBody(t *testing.T) {
+	t.Parallel()
+	// Older/partial server: detail omits body_document (text_snapshot empty).
+	// The seed must fall back to the list body rather than blanking it.
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		if _, err := io.WriteString(w, `{"trail":{"number":42},"checkpoints":[],"has_write_permission":true}`); err != nil {
+			t.Errorf("write response: %v", err)
+		}
+	}))
+	defer srv.Close()
+
+	client := api.NewClientWithBaseURL("tok", srv.URL)
+	found := &api.TrailResource{Number: 42, Body: "list body"}
+	body, err := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
+	if err != nil {
+		t.Fatalf("resolveTrailUpdateBody: %v", err)
+	}
+	if body != "list body" {
+		t.Fatalf("body = %q, want %q", body, "list body")
+	}
+}
+
+func TestResolveTrailUpdateBody_ReturnsErrorOnFetchFailure(t *testing.T) {
+	t.Parallel()
+	// A detail-fetch failure must be surfaced (not swallowed) so the caller can
+	// warn: a blank baseline could otherwise silently overwrite an unseen body.
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+	}))
+	defer srv.Close()
+
+	client := api.NewClientWithBaseURL("tok", srv.URL)
+	found := &api.TrailResource{Number: 42, Body: "list body"}
+	body, err := resolveTrailUpdateBody(t.Context(), client, "gh", "acme", "repo", found)
+	if err == nil {
+		t.Fatal("expected error on fetch failure, got nil")
+	}
+	if body != "list body" {
+		t.Fatalf("body = %q, want fallback %q", body, "list body")
+	}
+}
+
 func TestResolveCreateBranch(t *testing.T) {
 	t.Parallel()
 	tests := []struct {