Fix campfire messages --limit being ignored#367
Conversation
github-actions
bot
added
commands
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48f4a5630e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR fixes basecamp campfire/chat messages --limit being ignored by ensuring the CLI forwards the requested limit into the Basecamp SDK’s campfire line listing call, rather than fetching a default-sized page and trimming client-side afterward (which also previously selected the wrong end of the slice).
Changes:
- Forward
--limitto the SDK viaCampfireLineListOptions{Limit: ...}when listing campfire lines. - Remove the incorrect client-side post-fetch trimming logic.
- Add a regression test intended to validate
--limitbehavior forchat messages.
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.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/commands/chat.go | Passes --limit into ListLines and removes client-side slicing so the SDK controls pagination/truncation. |
| internal/commands/chat_test.go | Adds a new test + mock transport for validating chat messages --limit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
jeremy
force-pushed
the
fix-campfire-messages-limit
branch
from
48f4a56 to
d044b01
Compare
jeremy
force-pushed
the
fix-campfire-messages-limit
branch
from
d044b01 to
b6fd750
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
force-pushed
the
fix-campfire-messages-limit
branch
from
b6fd750 to
6ed6dd3
Compare
jeremy
force-pushed
the
fix-campfire-messages-limit
branch
from
6ed6dd3 to
6e48099
Compare
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Pass the user's --limit value to the SDK via CampfireLineListOptions instead of nil, so the SDK paginates to collect the requested number of messages. Remove the broken client-side trim that was both redundant and slicing the wrong end (oldest instead of newest). Fixes #335
jeremy
force-pushed
the
fix-campfire-messages-limit
branch
from
6e48099 to
47ae290
Compare
Cherry-picked fixes from pending main PRs: - Fix --assignee filter ignored with --list (#369) - Fix chat post JSON output contract and upload error handling (#365) - Fix schedule update silently dropping --attach without --description (#364) - Fix blank-line-after-list suppressing paragraph break (#363) - Fix campfire messages --limit being ignored (#367)
…accounts (#373) * Bump basecamp-sdk to v0.7.1 Fixes Person.Id deserialization (normalizeJSON pre-unmarshal for notification responses) and Account.Logo type (string → AccountLogo object). Adapts two breaking changes: Cards().Move() and Todos().Reposition() now take optional params. Adds AVIF/HEIC to richtext MIME extension map for logo upload support. * Merge upstream fixes (#363, #364, #365, #367, #369) Also fixes: chat upload error now wraps converted SDK error instead of raw error; schedule update --attach without --description now fetches current description to append rather than replace. * Add gauges command for project progress tracking Subcommands: list (account-wide), needles/needle (project-scoped), create/update/delete, enable/disable. Create validates --position (0-100), --color, --notify (requires --subscriptions for custom mode). * Add assignments command for viewing my assignments Subcommands: list, completed, due (with scope filter). Shortcut pattern — bare invocation shows priorities + non-priorities. * Add notifications command, extend accounts, register all new commands Notifications: list (paginated with dynamic next-page breadcrumb) and read (resolves IDs to SGIDs from --page). Accounts: show, update, logo upload (5MB + image type validation) and remove. Registers all new commands in root.go, catalog, and test harness. * Update docs and surface for v0.7.1 commands API-COVERAGE.md: 42 sections, 155 endpoints, SDK v0.7.1. SKILL.md: gauges, assignments, notifications, accounts sections. .surface: 148 commands, 422 flags.
Summary
--limitto the SDK viaCampfireLineListOptionsinstead ofnil, so pagination actually collects the requested number of messagesFixes #335. Also addresses the root cause of #284 (stale/old messages returned).
Test plan
TestChatMessagesLimit— mock returns 5 lines,--limit 3asserts 3 in outputbasecamp campfire messages --limit 50 --in <project>returns 50 messages (not 15)Summary by cubic
Fixes the campfire messages --limit flag being ignored and validates the flag. Returns the newest N messages with correct pagination and early stop.
--limitto the SDK viabasecamp.CampfireLineListOptions{Limit: ...}to collect the right count across pages and stop early.campfire messages --limitis silently capped at 15 — pagination not followed #335; addressescampfire messagesreturns stale/old messages instead of latest #284.Written for commit 47ae290. Summary will update on new commits.