Bound the account-wide listings, and drop the boost aggregate - #590
Open
jeremy wants to merge 5 commits into
Open
Bound the account-wide listings, and drop the boost aggregate#590jeremy wants to merge 5 commits into
jeremy wants to merge 5 commits into
Conversation
Soaking v0.8.0-rc.1 against a ~80-project account showed the account-wide listings are unusable at real scale. The cause is a contract decision, not a bug: I5 pinned each command's account-wide default to its project-scoped default. But project-scoped "all" is one project's items and account-wide "all" is the whole account, so the same word bought very different work. files list --all-projects HTTP 500 mid-crawl, no flag to recover with cards list --all-projects --status completed timed out at 94s (--page 1: 24s) checkins answers --all-projects timed out (one page: 1s) I5 also blessed "fetch page 0, then truncate", which made --limit useless exactly when it mattered: cards --limit 3 fetched all 2142 cards before trimming. Add accountWideCollect, one bounded walk over positive pages shared by all eight paths. It stops on an empty page, a page that adds no items, the cap, or the server's total. Two rules callers must know, so they are stated on the helper: Meta.TotalCount compares against len(items) — the top-level elements, which for a grouped payload are the groups, not the items nested inside — and the walk does not trim, so every caller cuts to the exact cap itself. Default to 100 for the paginated listings; --all is how you ask for the account. This changes the default for cards, checkins, forwards, and files. Project-scoped defaults are untouched. Give boost list and bare files list the --limit/--page/--all they never had. A listing with no escape hatch cannot recover from a server error mid-crawl, which is exactly how the files 500 became unrecoverable. Where a path has no pagination to thread — project-scoped files list, item-scoped boost list — it rejects all three by name rather than accepting and ignoring them. Both overdue paths now accept --all and reject --page. They are unpaginated, so --page addresses nothing, but capping at 100 while also rejecting --all left item 101 unreachable by any combination of flags. boost keeps a first-page default: /boosts.json?page=1 alone measured 93s, so walking toward 100 by default would multiply an already unacceptable wait. The exemption covers the default only — its flags walk like everywhere else — and its notices are per-mode, since "Showing first page" is false for --page 2. The slowness is server-side and tracked there. Dropping accountWideRespOpts's moreFlag parameter falls out of this: it existed so the two flagless commands would not be told to use an --all they did not have. They have one now.
The contract doc carried a hypothesis; production tracing settles it. Server timings for one page=40 request, 44,630ms total: Boost Load 22,097.6ms the paginated SELECT ~60 preloads <2.3ms each Boost Count 22,265.7ms GearedPagination's total-count query Completed 200 OK in 44630ms (Views: 244.7ms | ActiveRecord: 44370.9ms) 99.5% database time, in two statements of near-identical cost. That corrects the record twice over: the N+1 hypothesis is dead — the controller's preloads work, and 60 of 64 queries are trivial — and ordering the derived table, which the doc named as the suspect, is only half the story. The count pays the same materialization again for pagination metadata, so fixing the ORDER BY alone would leave roughly half the latency behind. The 93s figure was also wrong: that was the client giving up after three 30s attempts. A page costs ~44s, and page 40 costs the same as page 1 — the flat cost curve the materialization predicts. Since ~44s exceeds the SDK's 30s client timeout, boost list --all-projects does not merely run slow, it cannot complete. Filed as basecamp/bc3#12458, tracked alongside the files.json 500 (basecamp/bc3#12459) in basecamp-cli#589.
accountWideCollect checked the cap before exhaustion, so a listing whose total equalled the limit came back capped=true and the callers turned that into "more may exist" about a listing with nothing left in it. Test the known total first, and let the trim decide: holding every item still counts as capped when the caller is about to cut down to the cap. The exhaustion bound also read TotalCount from the current page while the helper documents the first page's as authoritative. A later page omitting X-Total-Count switched the bound off mid-walk and kept requesting past the declared end of the listing. messages and comments were discarding capped entirely, which made truncation silent on exactly the feeds that withhold a total — the case the notice exists for. messages also had a local `capped` meaning "this invocation applies a cap", which is a different question from "the walk stopped short"; it is now `bounded`, and the sorted path sets capped when its own trim removes anything.
The doc claimed the derived-table materialization as established. The tracing established the shape — two ~22s statements, 99.5% db time, N+1 disproved — but not the query plan behind it, and bc3#12458 correctly asks for an EXPLAIN rather than asserting one. Say the same thing here.
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: 2782cfa5a9
ℹ️ 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".
Comment on lines
+61
to
+64
| // accountWideDefaultLimit bounds every paginated account-wide listing that has | ||
| // no reason to differ. Project-scoped defaults are untouched: capping one | ||
| // project's items and capping the whole account are not the same promise. | ||
| const accountWideDefaultLimit = 100 |
There was a problem hiding this comment.
When these commands take the account-wide path, this new default caps results at 100, but their public flag descriptions still advertise --limit 0 as fetching all (cards.go:109, checkins.go:606, and forwards.go:68). Consequently, users following --help can receive only the first 100 records while expecting a complete listing; make the descriptions scope-aware or state that account-wide zero defaults to 100 and --all is required.
Useful? React with 👍 / 👎.
Comment on lines
+250
to
251
| if explicitLimit { | ||
| notice += " (raise or drop --limit for more)" |
There was a problem hiding this comment.
When an account contains more than an explicit limit above 100—for example, --limit 500 with 1,000 results—dropping the flag now restores the new 100-item default and returns fewer results, contrary to this notice. The same incorrect advice is duplicated for boosts at boost.go:332, where dropping a limit falls back to one page; both notices should recommend raising the limit or replacing it with --all.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR makes account-wide listing commands scale on large Basecamp accounts by replacing implicit full-account crawls with a shared bounded page-walk (default cap 100), and by adding missing pagination flags so operators can explicitly request more (or everything) when needed.
Changes:
- Introduces
accountWideCollectto bound account-wide fetches via positive pages, with consistent stop conditions and caller-side trimming. - Standardizes account-wide defaults to a cap of 100 (with the documented
boost listdefault exception) and updates notices to match bounded vs complete results. - Adds
--limit/-n,--page,--allto previously-flagless account-wide listings (files list,boost list) and rejects these flags on truly unpaginated scoped paths.
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 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Documents new bounded defaults and pagination flags for account-wide listings. |
| internal/commands/accountwide.go | Adds shared bounded collector + helpers, updates response option helper signature. |
| internal/commands/accountwide_test.go | Adds unit tests covering all accountWideCollect stop/guard semantics. |
| internal/commands/recording_test.go | Makes stub routes page-aware to support bounded-walk testing. |
| internal/commands/todos.go | Switches grouped todo account-wide collection to accountWideCollect; updates overdue --all behavior. |
| internal/commands/todos_test.go | Updates defaults and adds coverage for overdue --all behavior. |
| internal/commands/messages.go | Uses bounded collection for unsorted account-wide; preserves full-fetch for sorted mode; adjusts notices. |
| internal/commands/messages_test.go | Updates expectations for new default cap behavior and request sequencing. |
| internal/commands/comment.go | Switches account-wide comment collection to bounded walk + cap notice helper. |
| internal/commands/forwards.go | Bounds the default account-wide listing; supports explicit page/--all behavior. |
| internal/commands/forwards_test.go | Pins request sequences for bounded defaults via page-aware stubs. |
| internal/commands/checkins.go | Bounds default account-wide answers listing and adds cap notices consistent with bounded fetch. |
| internal/commands/checkins_test.go | Updates assertions for bounded behavior and new notice wording. |
| internal/commands/files.go | Adds pagination flags for account-wide files; rejects paging flags on project-scoped folder listing. |
| internal/commands/files_test.go | Adds coverage for new files pagination flags and project-scoped rejection behavior. |
| internal/commands/cards.go | Bounds default account-wide card listings; updates overdue behavior to accept --all and cap by default. |
| internal/commands/cards_test.go | Pins request sequences and adds tests for overdue --all semantics and updated notices. |
| internal/commands/boost.go | Adds pagination flags to account-wide boosts with first-page default; rejects paging flags for item-scoped boosts. |
| internal/commands/boost_test.go | Adds coverage for the four boost account-wide modes and new flags/rejections. |
| ACCOUNT-WIDE-LISTINGS.md | Updates the contract: bounded defaults, new pagination rules/flags, and documented exceptions. |
| .surface | Records the additive CLI surface changes for new flags across spellings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+159
to
+161
| if !capped || meta.TotalCount > count { | ||
| return "" | ||
| } |
The /boosts.json aggregate is an easter egg: unlinked from the Basecamp web UI, and served by a query that spends ~44s in MySQL on every page regardless of depth (basecamp/bc3#12458). It is being withdrawn server-side, so the CLI stops calling it instead of building a contract on top of that. boost list now requires an item ID. It loses --all-projects along with the --limit/--page/--all this branch had given it: an item's boosts arrive in one unpaginated response, and the SDK documents BoostListOptions.Page as not honoring a page number, so there was never anything for those flags to address once the aggregate went away. rejectScopedPaginationFlags is no longer needed here — flags that do not exist cannot be silently ignored. Everything().Boosts() stays in the SDK. Whether it should is the SDK's call; this repo just stops reaching for it, and API-COVERAGE.md now records 16 of the 17 aggregates as surfaced with the reason for the sixteenth. Seven commands list account-wide, not eight. On the surface baseline: --limit/--page/--all on boost were added and removed inside this unmerged branch, so they never shipped and recording them in .surface-breaking would claim a break that never happened. They are rewound out of the baseline instead. The two changes that did ship in v0.8.0-rc.1 -- `--all-projects`, and the argument going from optional to required -- are acknowledged as real removals. Net surface change against rc.1 is exactly those two.
jeremy
changed the title
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
internal/commands/accountwide.go:157
- The comment above accountWideCapNotice says it defers whenever the server reported a total, but the implementation actually defers only when
meta.TotalCount > count. Updating the comment to match the real behavior will avoid confusion for future readers.
// It defers to accountWideRespOpts whenever the server reported a total, since
// "N of M" is strictly more informative than "more may exist". The walk stops
// early by design, so on the feeds that withhold X-Total-Count the count is all
// there is to say.
internal/commands/accountwide.go:253
- When
meta.TotalCount > countand the user is on the default cap (no explicit --limit), the notice currently only suggests--all. That pushes users toward a potentially slow full crawl even though--limitis also a supported (and often cheaper) way to raise the cap.
notice += " (use --all for the complete list)"
internal/commands/accountwide.go:229
- This comment references "1 boosts" as the motivating example for singular/plural handling, but account-wide boosts have been removed in this PR. Consider making the wording generic so it stays accurate even as listings come and go.
// accountWideRespOpts builds the summary and truncation notice shared by the
// account-wide listings. singular/plural are the noun in both forms, since a
// one-item account-wide listing is common enough that "1 boosts" shows up.
internal/commands/accountwide.go:164
- accountWideCapNotice takes only a plural noun and will emit ungrammatical text for singular counts (e.g., "Showing the first 1 messages"). Consider changing the helper to accept both singular and plural (or otherwise handle singularization) and update callers accordingly.
return fmt.Sprintf(
"Showing the first %d %s; more may exist (use --all for every page, or --limit to raise the cap)",
count, plural)
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.
Soaking
v0.8.0-rc.1against a ~80-project production account showed the account-wide listings unusable at real scale. The cause was a contract decision rather than a bug:ACCOUNT-WIDE-LISTINGS.mdI5 pinned each command's account-wide default to its project-scoped default — but project-scoped "all" is one project's items and account-wide "all" is the whole account. The same word bought very different amounts of work.I5 also blessed "fetch page 0, then truncate", which made
--limituseless exactly when it mattered:cards list --limit 3fetched all 2142 cards before trimming.What changed
accountWideCollect— one bounded walk over positive pages, shared by all eight paths (todos,comment,messages,cards,checkins,forwards,files,boost). It stops on an empty page, a page that adds no items, the cap, or the server's declared total. Two rules callers must obey are stated on the helper:Meta.TotalCountcompares againstlen(items)— top-level elements, which on a grouped payload are the groups, never the inner-item counter.Uniform default cap of 100;
--allis how you ask for the account. Changes the default forcards,checkins,forwards,files. Project-scoped defaults untouched.Pagination flags for bare
files list—--limit/-n,--page,--all. A listing with no escape hatch cannot recover from a server error mid-crawl, which is exactly how thefiles500 became unrecoverable. Project-scopedfiles listhas no pagination to thread, so it rejects all three by name rather than accepting and ignoring them.The account-wide boost listing is withdrawn.
/boosts.jsonturned out to be an easter egg — unlinked from the web UI, and served by a query costing ~44s per page regardless of depth (basecamp/bc3#12458). It is being removed server-side, so the CLI stops calling it rather than building a contract on pathological database performance.boost listnow requires an item ID and carries no--all-projects/--limit/--page/--all. Seven commands list account-wide, not eight.Both overdue paths now accept
--alland reject--page. They are unpaginated, so--pageaddresses nothing; but capping at 100 and rejecting--allhad left item 101 unreachable by any combination of flags.Soak results (account 2914079)
cards list --all-projects --status completedcheckins answers --all-projectscards list --limit 3forwards list --all-projectscomments list --all-projectsmessages list --all-projectsNo default or
--limitrequest omitspage=any more.--allstill omits it deliberately — that is the spelling for the full traversal.What this does not fix
One listing remains broken for a server-side reason, filed and holding the release (basecamp-cli#589):
files.json500s on any page holding an attachment whose parent is an autolinked image. The bounded default does not dodge it: GearedPagination ramps page sizes (15, 30, 50, …), so the first 100 items span pages 1–4 and the walk lands on the failing page.The other blocker, basecamp/bc3#12458 (
boosts.jsonat ~44s/page), is resolved here by removing the caller — see above.Merging this is independent of #12459; the release is what waits on it.
Notes for review
accountWideRespOptslost itsmoreFlagparameter. It existed so the two flagless commands would not be advised to use an--allthey did not have — they have one now, and the linter caught the parameter going constant.stubRoute.pages). Without it a fixture serves the same body for every page number, so a bounded walk never sees an empty page and runs to the cap — which would have hidden the behavior under test.TotalCount-vs-len(items)rule, page-boundary overshoot, the exact-cap case (TotalCount == limitis complete, not truncated), and a total present only on page 1..surface: 21 additive lines (3 flags × 7files-family spellings). Boost contributes the only removals —--all-projectsand the argument going optional → required, both of which shipped inv0.8.0-rc.1, so both are acknowledged in.surface-breaking. The--limit/--page/--allthis branch briefly gave boost never shipped, so they are rewound out of the baseline rather than recorded as breaks that never happened. Net change against rc.1 is exactly the two real ones.Summary by cubic
Bound account-wide listings to a fast, bounded page walk with a 100-item default cap, replacing full-account crawls and making
--limiteffective. Removed the account-wide boosts feed due to extreme server-side latency;boost listis now item-scoped only and requires<id|url>.New Features
todos,comments,messages,cards,checkins,forwards, and account-widefiles; stops on empty page, no new items, cap, or known total.--allto traverse the whole account. Project-scoped defaults unchanged.--limit,--page,--allto account-widefiles list; project-scopedfiles listrejects these. Overdue listings accept--alland reject--page.Bug Fixes
--limitno longer fetches everything before trimming; callers trim after a page-boundary overshoot.Meta.TotalCounthandling, including totals present only on page 1; stop reporting truncation when total equals the limit.moreFlagparameter; tests and stub transport are page-aware to validate bounded walks.Written for commit 23a794a. Summary will update on new commits.