Add pagination to all SDK list methods (breaking, 0.4.0)#173
Conversation
Spec Change ImpactChanges:
SDK Impact:All SDKs need regeneration due to the breaking changes and updated pagination functionality. Checklist for SDK Updates:
|
There was a problem hiding this comment.
6 issues found across 79 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="go/pkg/basecamp/timeline_test.go">
<violation number="1" location="go/pkg/basecamp/timeline_test.go:101">
P2: Missing guard for negative `remaining` — if a page beyond the total is requested, `min(remaining, h.pageSize)` is negative and `make(…, count)` panics. Both other pagination handlers in this file include the guard.</violation>
</file>
<file name="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/services/BaseService.kt">
<violation number="1" location="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/services/BaseService.kt:251">
P2: This ~65-line method is nearly identical to `requestPaginated` above. Consider extracting the shared pagination loop into a private helper that both methods call, with `requestPaginated` discarding the first-page body. This avoids three copies of the same loop diverging over time.</violation>
</file>
<file name="go/pkg/basecamp/campfires.go">
<violation number="1" location="go/pkg/basecamp/campfires.go:229">
P2: Negative `Limit` is not normalized in `ListChatbots`, unlike every other list method in this file. If `opts.Limit` is -1, `limit` becomes -1 instead of 0 (unlimited). This works by accident today because `followPagination` uses `limit > 0` checks, but it's inconsistent and fragile.</violation>
</file>
<file name="kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/ServiceEmitter.kt">
<violation number="1" location="kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/ServiceEmitter.kt:400">
P3: Both branches of this conditional produce `","`, making it a no-op. Based on the analogous pattern in `generateMethod` (same file), the else branch should be `""`.</violation>
</file>
<file name="go/pkg/basecamp/todolist_groups.go">
<violation number="1" location="go/pkg/basecamp/todolist_groups.go:122">
P2: Doc says "non-zero" disables pagination, but the code checks `opts.Page > 0`. A negative `Page` (e.g. `-1`) silently falls through to full pagination instead of disabling it as documented. Either change the check to `opts.Page != 0` or update the doc to say "if positive".</violation>
</file>
<file name="go/pkg/basecamp/templates.go">
<violation number="1" location="go/pkg/basecamp/templates.go:124">
P2: Doc says "non-zero" but code checks `> 0`, so a negative `Page` value silently bypasses the single-page guard. Either update the comment to say "positive" or change the condition to `opts.Page != 0`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/services/BaseService.kt
Show resolved
Hide resolved
kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/ServiceEmitter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds Link-header pagination across all SDKs (plus wrapped-object pagination for endpoints like person progress), updates the Smithy/OpenAPI metadata to describe pagination behavior, and bumps SDK versions to 0.4.0 for breaking signature changes.
Changes:
- Add “wrapped pagination” support (preserve wrapper fields + paginate items under a key) in TS/Swift/Kotlin/Ruby runtimes and generators, with multi-page tests.
- Mark additional operations as paginated in Smithy/OpenAPI metadata and fix
GetPersonProgresspath handling to use the.jsonsuffix in generated SDKs. - Update Go SDK list-style methods to accept list options and follow Link pagination, plus version bumps across packages.
Reviewed changes
Copilot reviewed 56 out of 79 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/tests/services/base.test.ts | Adds a multi-page wrapped-pagination test for requestPaginatedWrapped. |
| typescript/src/services/base.ts | Implements requestPaginatedWrapped() and wrapped Link-following logic. |
| typescript/src/index.ts | Re-exports new generated options type for reports. |
| typescript/src/generated/services/timesheets.ts | Switches timesheet list methods to return paginated ListResult and adds entity aliases. |
| typescript/src/generated/services/timeline.ts | Updates timeline list return type to ListResult<TimelineEvent> and exports alias. |
| typescript/src/generated/services/reports.ts | Updates reports list returns to ListResult, adds wrapped pagination for personProgress, exports option types. |
| typescript/src/generated/schema.d.ts | Updates generated path types for .json person progress endpoint. |
| typescript/src/generated/path-mapping.ts | Updates operation mapping for .json person progress path. |
| typescript/src/generated/openapi-stripped.json | Regenerated stripped OpenAPI with pagination extensions and .json path key. |
| typescript/src/generated/metadata.json | Regenerated operation metadata including pagination descriptors. |
| typescript/src/client.ts | Bumps TypeScript SDK runtime version to 0.4.0. |
| typescript/scripts/generate-services.ts | Extends generator to support wrapped pagination (keyed array inside wrapper) and emit entity aliases. |
| typescript/package.json | Bumps package version to 0.4.0 and adjusts generate script input path. |
| typescript/package-lock.json | Updates lockfile for version bump. |
| swift/Tests/BasecampTests/PaginationTests.swift | Adds wrapped pagination accumulation test for personProgress. |
| swift/Sources/BasecampGenerator/TypeMapping.swift | Enhances entity resolution to handle wrapped pagination key. |
| swift/Sources/BasecampGenerator/ServiceEmitter.swift | Generates wrapped-pagination result structs and emits wrapped-pagination request code paths. |
| swift/Sources/BasecampGenerator/OpenAPIParser.swift | Parses pagination key extension for wrapped pagination operations. |
| swift/Sources/Basecamp/Services/BaseService.swift | Adds requestPaginatedWrapped and wrapped pagination follow logic with same-origin checks. |
| swift/Sources/Basecamp/Generated/Services/TimesheetsService.swift | Switches timesheet list methods to paginated results and adds maxItems options. |
| swift/Sources/Basecamp/Generated/Services/ReportsService.swift | Generates PersonProgressResult and uses wrapped pagination at runtime. |
| swift/Sources/Basecamp/BasecampConfig.swift | Bumps Swift SDK version to 0.4.0. |
| spec/basecamp.smithy | Marks additional operations as paginated and adds wrapped pagination key metadata. |
| spec/basecamp-traits.smithy | Extends basecampPagination trait with a key for wrapped responses. |
| scripts/enhance-openapi-go-types.sh | Adds an OpenAPI post-pass to rewrite the person progress path to include .json. |
| ruby/test/basecamp/services/timesheet_service_test.rb | Updates timesheet tests for new paginated/enumerator return behavior. |
| ruby/test/basecamp/services/reports_service_test.rb | Updates person progress path expectation and adds multi-page wrapped pagination test. |
| ruby/scripts/generate-services.rb | Extends Ruby generator to emit wrapped pagination methods and return types. |
| ruby/lib/basecamp/version.rb | Bumps Ruby SDK version to 0.4.0. |
| ruby/lib/basecamp/services/base_service.rb | Adds hook-safe wrapper for wrapped pagination enumerators and exposes paginate_wrapped. |
| ruby/lib/basecamp/http.rb | Implements paginate_wrapped for wrapped object pagination with SSRF protection. |
| ruby/lib/basecamp/generated/types.rb | Regenerated types header timestamp. |
| ruby/lib/basecamp/generated/services/timesheets_service.rb | Switches timesheet list methods to use pagination wrapper and return enumerators. |
| ruby/lib/basecamp/generated/services/reports_service.rb | Switches person progress to wrapped pagination and .json path. |
| ruby/lib/basecamp/generated/metadata.json | Regenerated Ruby SDK metadata including pagination descriptors. |
| ruby/lib/basecamp/client.rb | Exposes paginate_wrapped on account-scoped client. |
| ruby/Gemfile.lock | Updates gem version references to 0.4.0. |
| package.json | Bumps repo/root package version to 0.4.0. |
| openapi.json | Updates OpenAPI paths/extensions to reflect pagination and .json person progress path. |
| kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/PaginationTest.kt | Adds wrapped pagination accumulation test for person progress. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/services/BaseService.kt | Adds wrapped pagination request helper returning first-page body + accumulated items. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/timesheets.kt | Switches timesheets list methods to ListResult<TimesheetEntry> and decodes typed models. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/timeline.kt | Switches project timeline to ListResult<TimelineEvent>. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/reports.kt | Implements wrapped pagination for person progress and typed ListResult<TimelineEvent> for progress. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/Types.kt | Adds maxItems to relevant options types and conversion helper. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/models/TimesheetEntry.kt | Adds generated typed model for timesheet entries. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/models/TimelineEvent.kt | Adds generated typed model for timeline events. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/BasecampConfig.kt | Bumps Kotlin SDK version to 0.4.0. |
| kotlin/sdk/build.gradle.kts | Bumps Kotlin artifact version to 0.4.0. |
| kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/ServiceEmitter.kt | Adds wrapped pagination result class generation and wrapped pagination call emission. |
| kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/OperationParser.kt | Parses pagination key and passes it through operation model. |
| kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/OpenApiParser.kt | Enhances entity-type inference for wrapped pagination schema shapes. |
| kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/Config.kt | Adds type aliases for TimelineEvent/TimesheetEntry. |
| go/pkg/generated/client.gen.go | Updates generated request path for person progress to include .json. |
| go/pkg/basecamp/webhooks.go | Adds pagination-following behavior and new options/result types for listing webhooks. |
| go/pkg/basecamp/version.go | Bumps Go SDK version to 0.4.0. |
| go/pkg/basecamp/vaults.go | Adds pagination options/result for listing upload versions. |
| go/pkg/basecamp/todolist_groups.go | Adds pagination options/result for listing todolist groups. |
| go/pkg/basecamp/timesheet.go | Adds paginated results/options for project/recording timesheet reports. |
| go/pkg/basecamp/timeline_test.go | Updates tests for wrapped pagination behavior and new PersonProgress response shape. |
| go/pkg/basecamp/timeline.go | Adds/updates pagination handling across timeline endpoints, including custom wrapped pagination for person progress. |
| go/pkg/basecamp/templates.go | Adds pagination options/result for listing templates. |
| go/pkg/basecamp/search.go | Returns SearchListResult with pagination meta and follows pagination. |
| go/pkg/basecamp/people.go | Updates pingable listing to return list result + pagination support. |
| go/pkg/basecamp/message_types.go | Adds pagination options/result for listing message types. |
| go/pkg/basecamp/example_test.go | Updates examples to use new list result shapes. |
| go/pkg/basecamp/doc.go | Updates documentation snippet for new search return shape. |
| go/pkg/basecamp/client_replies.go | Adds pagination options/result for listing client replies. |
| go/pkg/basecamp/client_correspondences.go | Adds pagination options/result for listing client correspondences. |
| go/pkg/basecamp/client_approvals.go | Adds pagination options/result for listing client approvals. |
| go/pkg/basecamp/campfires_test.go | Updates tests for new list method signatures with options param. |
| go/pkg/basecamp/campfires.go | Adds pagination options/results for multiple campfire list endpoints plus defaults. |
| go/pkg/basecamp/boosts_test.go | Updates tests for new list method signatures with options param. |
| go/pkg/basecamp/boosts.go | Adds pagination options/result for listing boosts. |
| conformance/tests/paths.json | Updates conformance expectations for .json person progress path. |
| conformance/runner/typescript/package-lock.json | Updates runner lockfile for linked SDK (currently inconsistent with 0.4.0 bump). |
| conformance/runner/ruby/runner.rb | Adapts runner for timesheets returning enumerators. |
| conformance/runner/go/main.go | Adapts runner for Go list methods’ new options parameter. |
| behavior-model.json | Adds pagination metadata for newly paginated operations. |
Files not reviewed (2)
- conformance/runner/typescript/package-lock.json: Language not supported
- typescript/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
basecamp-sdk/typescript/src/client.ts
Line 301 in a61b01f
createBasecampClient exposes requestTimeoutMs, and normal requests get that timeout via createAuthMiddleware, but follow-up page fetches call bare fetch(url, { headers }) with no AbortSignal. When a list endpoint has a Link header and page 2+ stalls, the operation can hang indefinitely instead of timing out like page 1, which is a production regression for paginated methods.
ℹ️ 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".
|
Addressed all bot review findings: Fixed (5):
Declined with rationale (4):
Out of scope (1):
|
Add `key` field to `basecampPagination` trait for wrapped-object pagination
(responses like `{person, events}` where paginated items are nested under a
key rather than returned as a bare array).
Mark GetProjectTimesheet and GetRecordingTimesheet as paginated. Fix
GetPersonProgress URI to include `.json` suffix via enhance-openapi script.
Regenerate openapi.json, client.gen.go, and behavior-model.json.
Each SDK gains the ability to paginate responses where items are nested
inside a wrapper object (e.g. `{person: {...}, events: [...]}`) rather
than returned as a bare array.
Ruby: `paginate_wrapped` helper + `wrap_paginated_wrapped` hook wrapper
that fires on_operation_start eagerly and on_operation_end when the lazy
Enumerator completes.
TypeScript: `requestPaginatedWrapped<K, TItem>` method preserving wrapper
fields alongside a `ListResult<TItem>` for the paginated key.
Swift: `requestPaginated` overload accepting `itemsKey` parameter to
extract items from wrapped responses.
Kotlin: Generator emits custom `parseItems` lambda extracting items from
the wrapper object's key.
All four generators updated to read `x-basecamp-pagination.key` and route
to the wrapped pagination path.
Regenerate Ruby, TypeScript, Swift, and Kotlin services from updated OpenAPI spec. PersonProgress now uses wrapped pagination (key: "events"), and timesheet for_project/for_recording are now paginated. Add multi-page wrapped pagination tests exercising the full person→events accumulation path in all five SDKs (Go test in next commit). Each test verifies wrapper fields (person) are preserved while events paginate across Link-header pages.
21 list methods previously returned only page 1. Each now accepts
optional list options (Limit, Page) and follows Link-header pagination
via followPagination, returning *ListResult with TotalCount and Truncated
metadata.
Group A (12 methods): webhooks, client approvals/correspondences/replies,
templates, campfires, campfire lines/uploads, boosts, todolist groups,
message types — gain ListOptions parameter and followPagination.
Group B (7 methods): progress, project timeline, person progress, search,
pingable people, vault versions, chatbots — gain result wrapper types.
Group C (2 methods): project/recording timesheet reports — gain
pagination on existing TimesheetReportOptions.
PersonProgress uses a custom wrapped pagination loop since each page
returns {person, events} rather than a bare array. Includes multi-page
wrapped pagination tests.
BREAKING: All 21 methods have new signatures (added opts parameter
and/or changed return types). Callers pass nil for default behavior.
Adapt Go, Ruby, and TypeScript conformance runners for new pagination signatures. Update paths.json for .json-suffixed person progress route. Bump version to 0.4.0 across Go, Ruby, and root package.json to reflect breaking pagination API changes.
Fix doc/code mismatch: docs said "Use -1 for unlimited" but code treats any negative value as unlimited. Update all three method docs to say "negative = unlimited" matching the actual behavior. Add default-cap (Limit:0 and nil with >100 items) and unlimited tests for ProjectTimeline and PersonProgress, achieving symmetric test coverage across all three timeline entrypoints.
- Add missing negative-remaining guard in wrappedPaginationHandler test helper, matching the other two pagination handlers - Normalize negative Limit in ListChatbots to match all other list methods (was passing raw negative value through) - Fix Kotlin generator trailing comma no-op: else branch now emits "" instead of "," for last property in data class - Update Page field docs from "non-zero" to "positive" across all files changed in this PR, matching the actual `> 0` check - Regenerate Kotlin services (fixes trailing comma in PersonProgressResult)
Strip newlines from test result messages before printing to prevent log injection (go/log-injection). Pre-existing code newly flagged by CodeQL due to surrounding changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 79 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- conformance/runner/typescript/package-lock.json: Language not supported
- typescript/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Kotlin generator produces Int (matching openapi.json type: integer). The Double was from a stale regeneration and doesn't match the spec.
Summary
keyfield tobasecampPaginationtrait for wrapped-object pagination ({person, events}). MarkGetProjectTimesheetandGetRecordingTimesheetas paginated. FixGetPersonProgressURI suffix.person) alongside paginated items.personProgressuses wrapped pagination; timesheetfor_project/for_recordingare now paginated. Multi-page wrapped pagination tests in all 5 SDKs.*ListOptions{Limit, Page}and follow Link-header pagination.PersonProgressuses a custom wrapped loop matching the real BC3 API shape.Breaking changes
Go: 21 methods gain a new options parameter and/or change return types to
*ListResult. Callers: passnilfor default behavior.Ruby:
person_progressreturns a Hash where"events"is a lazyEnumeratorspanning all pages (was: page-1 JSON only).TypeScript:
personProgress()returns wrapped type withevents: ListResult<TimelineEvent>.progress()andprojectTimeline()returnListResult<TimelineEvent>.Swift:
personProgress()returnsListResult<TimelineEvent>embedded in a wrapper.Kotlin:
personProgress()returns wrapped result with paginated events.Test plan
makepasses end-to-end (lint + all SDK tests + conformance)TestPersonProgress_MultiPageWrapped,TestPersonProgress_MultiPageWithLimittest_person_progress_multi_page_wrappedrequestPaginatedWrappedtest suitetestWrappedPaginationAccumulatesAcrossPageswrappedPaginationAccumulatesAcrossPagesSummary by cubic
Adds link‑header pagination to all SDK list methods, including wrapped-object pagination for
{person, events}. Timesheets and person progress now paginate, the person progress path uses.json, negative timeline limits mean unlimited, and all SDKs are bumped to 0.4.0 with breaking signatures.New Features
keytobasecampPaginationfor wrapped responses; marked timesheets as paginated; fixedGetPersonProgresspath to/reports/users/progress/{personId}.json; regenerated OpenAPI/metadata.PersonProgresspreservespersonand paginatesevents; timeline/timesheets return typedListResult(TimelineEvent,TimesheetEntry); Go switched 21 list methods to accept options and returnListResultwith meta; docs clarify “Page: positive” and any negativeLimitis unlimited; conformance runners updated and Go runner sanitizes log output; Kotlin fix: revertUploadwidth/heighttoInt.Migration
nilfor defaults; read items from the returned wrapper (e.g.,Results,Entries) and useMetafor counts;Search,timeline, and timesheets returnListResult.reports.person_progressreturns a Hash with"events"as a lazy Enumerator;timesheets.for_project/for_recordingnow return an Enumerator.@37signals/basecamp):reports.personProgress()returns{ person, events: ListResult<TimelineEvent> };timeline.projectTimeline()andtimesheetsreturnListResult<TimesheetEntry>.Reports.personProgress()returnsPersonProgressResultwithpersonandevents: ListResult<TimelineEvent>;Timesheetsmethods returnListResult<TimesheetEntry>.reports.personProgress()returnsPersonProgressResultwith wrapped events;timeline.projectTimeline()andtimesheetsreturnListResult;Uploadwidth/heightareInt.Written for commit 2e406db. Summary will update on new commits.