fix(attachments): download real bytes and hint --type for bucket-scoped recordings#447
Open
nnemirovsky wants to merge 2 commits intobasecamp:mainfrom
Open
fix(attachments): download real bytes and hint --type for bucket-scoped recordings#447nnemirovsky wants to merge 2 commits intobasecamp:mainfrom
nnemirovsky wants to merge 2 commits intobasecamp:mainfrom
Conversation
ParsedAttachment.DisplayURL() returned the preview URL
(preview.3.basecamp.com/.../previews/full) before the download href
(storage.3.basecamp.com/.../download/<filename>). 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.
There was a problem hiding this comment.
Pull request overview
Fixes attachment downloading for non-image files by ensuring the CLI uses the real download URL (blob) instead of the preview URL, and improves the error experience when generic recording lookups can’t resolve bucket-scoped items (notably Kanban cards).
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Update
ParsedAttachment.DisplayURL()to preferHref(download endpoint) overURL(preview endpoint). - Adjust
fetchItemContentto surface a--typeusage hint when the generic/recordings/<id>.jsonlookup returns 404. - Update/add tests to lock in
Hrefpreference and the new 404-to-hint behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/richtext/richtext.go | Prefer downloadable Href over preview URL for attachment URLs. |
| internal/richtext/richtext_test.go | Update DisplayURL expectations (Href wins; URL fallback). |
| internal/commands/attachments.go | Add 404 handling on generic lookup and share the hint string via a const. |
| internal/commands/attachments_test.go | Update attachment meta expectations and add test for 404 generic lookup hinting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The generic /recordings/<id>.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/<bucket>/card_tables/cards/<id>.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.
f356e02 to
a284543
Compare
nnemirovsky
added a commit
to nnemirovsky/basecamp-cli
that referenced
this pull request
Apr 24, 2026
Combines both PRs' branches for local use via `go install`: - PR basecamp#421 (trix): <div>/<div><br></div> paragraph shape, soft→hard break in top-level paragraphs, HTMLToMarkdown round-trip for <div>. - PR basecamp#447 (attachments): prefer download Href over preview URL in ParsedAttachment.DisplayURL; suggest --type on 404 from generic /recordings lookup. Upstream each PR remains separately reviewable at basecamp/basecamp-cli.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related fixes so
basecamp attachments download <card>andbasecamp cards show --download-attachments=...actually retrieve the file bytes for non-image attachments on Kanban cards.Repro
Attach a non-image file (e.g. a small
report.csv) to a card's description, then try to download:Image attachments worked by coincidence, which is why this went unnoticed.
Root causes
1.
ParsedAttachment.DisplayURL()returned the preview URL instead of the download href.Every
<bc-attachment>element carries both aurl(preview.3.basecamp.com/.../previews/full) and anhref(storage.3.basecamp.com/.../download/<filename>).DisplayURL()preferredurl. For non-image content types the preview endpoint returns a generic SVG file-type icon rather than the real blob, so every caller downstream (cards show --download-attachments,attachments list,attachments download,downloadableAttachments) silently got the wrong bytes with the right filename.Fix:
DisplayURL()now prefersHrefand falls back toURL. All internal callers are download paths, so the flip is safe.2.
fetchItemContentreturned a bare "Resource not found" for cards.The generic
/recordings/<id>.jsonendpoint is not addressable at the account scope for Kanban::Card — the recording is only reachable via the bucket-scoped/buckets/<bucket>/card_tables/cards/<id>.json. The function already has a friendly "Specify a type" usage hint for the 204 branch but routed 404s throughconvertSDKError, hiding the real cause.Fix: on 404 from the generic path, when no
--typewas provided, emit the same usage hint. Narrow — only fires whenisGenericType(recordType)and the SDK error isCodeNotFound. Any path that already worked is untouched. The hint string is now a package const shared between the 204 and 404 branches so they cannot drift.Test coverage gap this exposed
internal/richtext/richtext_test.gohad a"URL wins"case, andinternal/commands/attachments_test.go'sTestAttachmentMeta "uses DisplayURL"asserted the same. Both pinned the buggy behavior. Updated to assertHrefwins over previewURL, with a separate case for theURL-only fallback path.Added
TestAttachmentsList404GenericPathSuggestsTypecovering the 404 →--typehint conversion, mirroring the existingTestFetchItemContentRefetchesLineWithLargeParentIDfixture style.Commits
fix(richtext): prefer Href over preview URL in DisplayURLfix(attachments): suggest --type on 404 from generic recording lookupLocal verification
make fmt-checkcleango vet ./...cleanmake lint(golangci-lint) 0 issuesgo test ./...all packages greenSummary by cubic
Fixes attachment downloads to return the actual file bytes and adds a
--typehint for bucket‑scoped recordings (e.g., cards). Impactsbasecamp cards show --download-attachments,basecamp attachments list, andbasecamp attachments download.Href(download link) over previewURLinParsedAttachment.DisplayURL()so non-image attachments fetch the real file;URLremains a fallback./recordings/<id>.jsonwith no--type, show a "Specify a type" hint to use the typed endpoint (needed for cards and similar); do not show the hint if a type was provided.Written for commit a284543. Summary will update on new commits.