From 3560d7d33d3ef5f8f788a80af24231983a59c967 Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Fri, 24 Apr 2026 12:48:09 +0800 Subject: [PATCH 1/2] fix(richtext): prefer Href over preview URL in DisplayURL ParsedAttachment.DisplayURL() returned the preview URL (preview.3.basecamp.com/.../previews/full) before the download href (storage.3.basecamp.com/.../download/). For non-image attachments (csv, xlsx, zip, pdf, ...) the preview endpoint returns a generic SVG file-type icon, so every caller that went through DisplayURL() downloaded a ~3.8KB placeholder instead of the real file. Image attachments happened to work because their preview URL returns the real image, which is why this bug went unnoticed. Every internal caller of DisplayURL() is a download path (cards show --download-attachments, attachments list, attachments download, downloadableAttachments), so preferring Href is correct across the board. URL is kept as a fallback for the rare case where an attachment has no downloadable blob. The existing richtext unit test pinned the wrong behavior ("URL wins") and the commands TestAttachmentMeta fixture did the same; both are updated to assert the new contract with a non-image-looking URL pair. --- internal/commands/attachments_test.go | 8 ++++---- internal/richtext/richtext.go | 13 ++++++++++--- internal/richtext/richtext_test.go | 5 +++-- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index 7a7c9a67..a102ac29 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -300,16 +300,16 @@ func TestWithAttachmentMeta(t *testing.T) { func TestAttachmentMeta(t *testing.T) { t.Run("uses DisplayURL", func(t *testing.T) { atts := []richtext.ParsedAttachment{ - {URL: "https://example.com/a.png", Href: "https://example.com/a-href.png", Filename: "a.png", ContentType: "image/png", Filesize: "1024"}, - {Href: "https://example.com/b.txt"}, + {URL: "https://preview.example.com/a-preview", Href: "https://storage.example.com/download/a.png", Filename: "a.png", ContentType: "image/png", Filesize: "1024"}, + {URL: "https://preview.example.com/b-preview"}, } result := attachmentMeta(atts, nil) assert.Len(t, result, 2) assert.Equal(t, "a.png", result[0]["filename"]) assert.Equal(t, "image/png", result[0]["content_type"]) assert.Equal(t, "1024", result[0]["filesize"]) - assert.Equal(t, "https://example.com/a.png", result[0]["url"]) - assert.Equal(t, "https://example.com/b.txt", result[1]["url"]) + assert.Equal(t, "https://storage.example.com/download/a.png", result[0]["url"]) + assert.Equal(t, "https://preview.example.com/b-preview", result[1]["url"]) _, hasFilename := result[1]["filename"] assert.False(t, hasFilename) }) diff --git a/internal/richtext/richtext.go b/internal/richtext/richtext.go index cf7b4474..65557443 100644 --- a/internal/richtext/richtext.go +++ b/internal/richtext/richtext.go @@ -1248,9 +1248,16 @@ func (a *ParsedAttachment) DisplayName() string { } // DisplayURL returns the best available URL for the attachment. +// Href is preferred because it points at the real blob download endpoint +// (storage.3.basecamp.com/.../download/). URL points at the +// preview endpoint (preview.3.basecamp.com/.../previews/full), which for +// non-image content types returns a generic SVG file-type icon instead of +// the real file. Every internal caller is a download path, so preferring +// Href is correct; URL is retained as a fallback for the rare case where +// an attachment has no downloadable blob (e.g. externally hosted images). func (a *ParsedAttachment) DisplayURL() string { - if a.URL != "" { - return a.URL + if a.Href != "" { + return a.Href } - return a.Href + return a.URL } diff --git a/internal/richtext/richtext_test.go b/internal/richtext/richtext_test.go index c1881bec..1e468fd8 100644 --- a/internal/richtext/richtext_test.go +++ b/internal/richtext/richtext_test.go @@ -2200,8 +2200,9 @@ func TestParsedAttachmentDisplayURL(t *testing.T) { att ParsedAttachment expected string }{ - {"URL wins", ParsedAttachment{URL: "https://a.com", Href: "https://b.com"}, "https://a.com"}, - {"href fallback", ParsedAttachment{Href: "https://b.com"}, "https://b.com"}, + {"Href wins over preview URL", ParsedAttachment{URL: "https://preview.example.com/icon", Href: "https://storage.example.com/download/file.csv"}, "https://storage.example.com/download/file.csv"}, + {"URL fallback when Href missing", ParsedAttachment{URL: "https://preview.example.com/icon"}, "https://preview.example.com/icon"}, + {"Href only", ParsedAttachment{Href: "https://storage.example.com/download/file.csv"}, "https://storage.example.com/download/file.csv"}, {"empty", ParsedAttachment{}, ""}, } for _, tt := range tests { From a2845438ec5d82b9c98c4f6aa21897fe24f65307 Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Fri, 24 Apr 2026 12:48:36 +0800 Subject: [PATCH 2/2] fix(attachments): suggest --type on 404 from generic recording lookup The generic /recordings/.json endpoint is not addressable at the account scope for every recording type. Kanban::Card is the concrete case: its recording is only reachable via the bucket-scoped /buckets//card_tables/cards/.json endpoint, so the generic lookup returns 404. Before this commit, fetchItemContent routed that 404 through convertSDKError and surfaced a bare "Resource not found" error, leaving the user with no hint that --type was required. The existing 204 branch of the same function already emits a friendly "Specify a type" usage hint listing every accepted type. Do the same on 404 from the generic path so cards (and any other bucket-scoped type) produce the actionable error. Narrow scope: only fires when isGenericType(recordType) and the SDK error is CodeNotFound, so any path that already worked is untouched. Pulls the hint string into a package const (genericLookupTypeHint) so the 204 and 404 branches stay in sync. --- internal/commands/attachments.go | 49 ++++++++++++++++- internal/commands/attachments_test.go | 76 +++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/internal/commands/attachments.go b/internal/commands/attachments.go index 2e428119..71b0f417 100644 --- a/internal/commands/attachments.go +++ b/internal/commands/attachments.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -16,12 +17,26 @@ import ( "charm.land/lipgloss/v2" "github.com/spf13/cobra" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" "github.com/basecamp/basecamp-cli/internal/urlarg" ) +// genericLookupTypeHint is the usage hint emitted when the generic +// /recordings/.json lookup cannot resolve the recording — either +// because the endpoint returns 204 (some recording types) or 404 (cards, +// and any other type whose recording is not addressable without bucket +// scope). Callers need to specify --type (or pass a URL, from which the +// type is parsed) so the typed endpoint can be used instead. +// +// Kept command-agnostic: fetchItemContent is shared by `attachments list` +// and `attachments download`, so the hint must not tell a download caller +// to re-run the list command (or vice versa). +const genericLookupTypeHint = "Re-run with --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|answer|forward|upload, or pass a URL (which encodes the type)" + // NewAttachmentsCmd creates the attachments command group. func NewAttachmentsCmd() *cobra.Command { cmd := &cobra.Command{ @@ -401,6 +416,20 @@ func isGenericType(recordType string) bool { } } +// shouldSuggestType reports whether the --type usage hint is appropriate +// for the current recordType. It fires only when no explicit type was +// provided. The "line"/"lines"/"replies" aliases route through the same +// generic /recordings lookup for parent discovery, but the user *did* +// pass --type — telling those callers to "specify --type" is misleading. +func shouldSuggestType(recordType string) bool { + switch recordType { + case "", "recording", "recordings": + return true + default: + return false + } +} + // fetchItemContent retrieves the HTML content field from a Basecamp item. // Uses the same recording-type discovery pattern as show.go. func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string) (string, error) { @@ -416,13 +445,29 @@ func fetchItemContent(cmd *cobra.Command, app *appctx.App, id, recordType string resp, err := app.Account().Get(cmd.Context(), endpoint) if err != nil { + // The generic /recordings/.json endpoint returns 404 for + // recording types that require bucket scope (e.g. Kanban::Card). + // Convert to the same usage hint the 204 branch below emits so + // the user is told to pass --type instead of seeing a bare + // "Resource not found". Only fires when no explicit type was + // provided — a 404 under --type line|replies means the ID is + // wrong, not that --type is missing. + if shouldSuggestType(recordType) { + var sdkErr *basecamp.Error + if errors.As(err, &sdkErr) && sdkErr.Code == basecamp.CodeNotFound { + return "", output.ErrUsageHint( + fmt.Sprintf("Item %s not found or type required", id), + genericLookupTypeHint, + ) + } + } return "", convertSDKError(err) } if resp.StatusCode == http.StatusNoContent { - if isGenericType(recordType) { + if shouldSuggestType(recordType) { return "", output.ErrUsageHint( fmt.Sprintf("Item %s not found or type required", id), - "Specify a type: basecamp attachments list --type todo|todolist|message|comment|card|card-table|document|schedule-entry|checkin|answer|forward|upload", + genericLookupTypeHint, ) } return "", output.ErrNotFound("item", id) diff --git a/internal/commands/attachments_test.go b/internal/commands/attachments_test.go index a102ac29..2816a1cf 100644 --- a/internal/commands/attachments_test.go +++ b/internal/commands/attachments_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -13,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/richtext" ) @@ -509,3 +511,77 @@ func TestFetchItemContentRefetchesLineWithLargeParentID(t *testing.T) { assert.Contains(t, reqs[0], "/recordings/111.json") assert.Contains(t, reqs[1], "/chats/"+largeID+"/lines/111.json") } + +// TestAttachmentsList404GenericPathSuggestsType verifies that when the +// generic /recordings/.json endpoint returns 404 and no --type is +// provided, the CLI surfaces the "Specify a type" usage hint rather than +// a bare "Resource not found". Cards are the concrete case: their +// recording is only addressable via the bucket-scoped card_tables +// endpoint, so the generic lookup returns 404. +func TestAttachmentsList404GenericPathSuggestsType(t *testing.T) { + const cardID = "12345" + + transport := &showTrackingTransport{ + responder: func(path string) (int, string) { + if strings.Contains(path, "/recordings/"+cardID) { + return 404, `{"error":"not found"}` + } + return 200, `{}` + }, + } + app := showTestApp(t, transport) + + cmd := NewAttachmentsCmd() + cmd.SetArgs([]string{"list", cardID}) + ctx := appctx.WithApp(context.Background(), app) + cmd.SetContext(ctx) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := cmd.Execute() + require.Error(t, err) + + var e *output.Error + require.True(t, errors.As(err, &e), "expected output.Error, got %T: %v", err, err) + assert.Contains(t, e.Message, cardID) + assert.Contains(t, e.Message, "type required") + assert.Contains(t, e.Hint, "--type") + assert.Contains(t, e.Hint, "card") +} + +// TestAttachmentsList404WithExplicitTypeDoesNotSuggestType verifies the +// hint gate is narrow: when the user already passed --type line (or any +// non-suggest-appropriate type that routes through the generic /recordings +// lookup for parent discovery), a 404 must not produce the "Specify a +// type" hint — they already did. +func TestAttachmentsList404WithExplicitTypeDoesNotSuggestType(t *testing.T) { + const lineID = "12345" + + transport := &showTrackingTransport{ + responder: func(path string) (int, string) { + if strings.Contains(path, "/recordings/"+lineID) { + return 404, `{"error":"not found"}` + } + return 200, `{}` + }, + } + app := showTestApp(t, transport) + + cmd := NewAttachmentsCmd() + cmd.SetArgs([]string{"list", lineID, "--type", "line"}) + ctx := appctx.WithApp(context.Background(), app) + cmd.SetContext(ctx) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + err := cmd.Execute() + require.Error(t, err) + + var e *output.Error + if errors.As(err, &e) { + assert.NotContains(t, e.Hint, "--type", + "line callers already provided --type; hint would be misleading") + assert.NotContains(t, e.Message, "type required", + "should not claim type is required when it was provided") + } +}