Absorb the Bubble Ups successor surface (bc3 #11628)#426
Conversation
github-actions
Bot
added
typescript
Spec Change Impact
Impact Analysis:
Checklist of SDKs for Updates:
|
Review carefully before merging. Consider a major version bump. |
![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: 277533db15
ℹ️ 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".
| * const result = await client.myNotifications.bubbleUps(); | ||
| * ``` | ||
| */ | ||
| async bubbleUps(options?: BubbleUpsMyNotificationOptions): Promise<components["schemas"]["GetBubbleUpsResponseContent"]> { |
There was a problem hiding this comment.
For TypeScript callers, this declaration resolves to a plain Notification[], even though requestPaginated returns a ListResult and the method documentation promises .meta.totalCount; consequently, valid code such as result.meta.totalCount is rejected by TypeScript. Update the service generator's entity/type mapping so it emits ListResult<Notification>, then regenerate this file rather than patching generated output directly.
AGENTS.md reference: AGENTS.md:L68-L72
Useful? React with 👍 / 👎.
| @basecampRetry(maxAttempts: 3, baseDelayMs: 1000, backoff: "exponential", retryOn: [429, 503]) | ||
| @basecampPagination(style: "link", totalCountHeader: "X-Total-Count", maxPageSize: 50) | ||
| @http(method: "GET", uri: "/{accountId}/my/readings/bubble_ups.json") | ||
| operation GetBubbleUps { |
There was a problem hiding this comment.
This introduces a new operation, but a repo-wide search finds only Go wrapper tests and the dormant conformance descriptor—there are no happy-path and error-case service tests for the generated TypeScript, Ruby, or Python methods. Add those tests so each SDK's path, pagination, and error behavior is verified before the operation ships.
AGENTS.md reference: AGENTS.md:L299-L308
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR absorbs the BC5 “Bubble Ups” successor surface into the Smithy/OpenAPI spec and fans it out across all SDKs: adds required bubble-up counts + an optional limit_bubble_ups query to GetMyNotifications, and introduces a new paginated GetBubbleUps operation.
Changes:
- Add required
bubble_ups_count/scheduled_bubble_ups_counttoGetMyNotificationsoutput and optionallimit_bubble_upsquery param to its input. - Add new
GetBubbleUps(GET /my/readings/bubble_ups.json) as a Link-header paginated bare-array operation, plus corresponding SDK methods. - Update conformance validators/canary and register the absorption in
spec/api-gaps.
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 11 out of 29 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/src/generated/services/my-notifications.ts | Adds limitBubbleUps option and new bubbleUps() paginated method (typing/doc issues noted in review). |
| typescript/src/generated/schema.d.ts | Extends TS schema with GetBubbleUps + required count fields on GetMyNotifications. |
| typescript/src/generated/path-mapping.ts | Maps the new bubble-ups route to GetBubbleUps. |
| typescript/src/generated/openapi-stripped.json | Includes new endpoint and required count fields in the stripped OpenAPI artifact. |
| typescript/src/generated/metadata.ts | Adds retry/pagination metadata for GetBubbleUps. |
| swift/Sources/Basecamp/Generated/Services/MyNotificationsService.swift | Adds Swift bubbleUps() and limitBubbleUps query support. |
| swift/Sources/Basecamp/Generated/Models/GetMyNotificationsResponseContent.swift | Makes bubble-up counts required in the Swift response model. |
| swift/Sources/Basecamp/Generated/Metadata.swift | Registers retry config for GetBubbleUps. |
| spec/overlays/tags.smithy | Tags GetBubbleUps into the MyNotifications service group. |
| spec/basecamp.smithy | Adds GetBubbleUps, the limit_bubble_ups query param, and required count fields. |
| spec/api-gaps/README.md | Adds bubble-ups-surface entry to the registry index. |
| spec/api-gaps/bubble-ups-surface.md | New registry entry documenting the absorbed bubble-ups surface. |
| ruby/lib/basecamp/generated/types.rb | Regeneration timestamp bump from OpenAPI changes. |
| ruby/lib/basecamp/generated/services/my_notifications_service.rb | Adds limit_bubble_ups param and new get_bubble_ups paginated enumerator. |
| ruby/lib/basecamp/generated/metadata.json | Adds retry/pagination metadata for GetBubbleUps (Ruby artifact). |
| python/src/basecamp/generated/types.py | Adds required bubble-up count fields to the typed response dict. |
| python/src/basecamp/generated/services/my_notifications.py | Adds limit_bubble_ups param and new get_bubble_ups (sync + async). |
| python/src/basecamp/generated/metadata.json | Adds retry metadata for GetBubbleUps (Python artifact). |
| openapi.json | Adds GetBubbleUps path and the limit_bubble_ups + required count fields to the main OpenAPI spec. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/Types.kt | Adds GetBubbleUpsOptions and limitBubbleUps for notifications options. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/my-notifications.kt | Adds Kotlin bubbleUps() and limit_bubble_ups query support. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/Metadata.kt | Registers retry config for GetBubbleUps. |
| go/pkg/generated/client.gen.go | Adds generated client support for GetBubbleUps and new count fields/params. |
| go/pkg/basecamp/url-routes.json | Adds route mapping for GetBubbleUps in Go URL routes. |
| go/pkg/basecamp/my_notifications.go | Adds Go wrapper BubbleUps() and WithLimitBubbleUps() option on Get. |
| go/pkg/basecamp/my_notifications_test.go | Adds Go tests for limit_bubble_ups decode and multi/single-page pagination behavior. |
| conformance/tests/live-my-surface.json | Adds a dormant live-schema validation canary entry for GetBubbleUps. |
| conformance/runner/typescript/schema-validator.test.ts | Updates TS schema validator fixture to include newly-required counts. |
| behavior-model.json | Adds behavior metadata for GetBubbleUps (readonly, retry, pagination). |
💡 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}
There was a problem hiding this comment.
1 issue found across 29 files
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="spec/api-gaps/bubble-ups-surface.md">
<violation number="1" location="spec/api-gaps/bubble-ups-surface.md:8">
P3: Two of the three `smithy_refs` line numbers in `bubble-ups-surface.md`'s frontmatter are inaccurate — `limit_bubble_ups` is off by 14 lines and `bubble_ups_count`/`scheduled_bubble_ups_count` is off by 7–13 lines. Since this file is being added fresh in this PR, these references should reflect the final state of the spec file. Consider updating the line numbers to match the current positions so the references remain useful navigation aids.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| sdk_demand: high | ||
| bc3_pr: 11628 | ||
| smithy_refs: | ||
| - "GetMyNotificationsOutput.bubble_ups_count/scheduled_bubble_ups_count (spec/basecamp.smithy:8577)" |
There was a problem hiding this comment.
P3: Two of the three smithy_refs line numbers in bubble-ups-surface.md's frontmatter are inaccurate — limit_bubble_ups is off by 14 lines and bubble_ups_count/scheduled_bubble_ups_count is off by 7–13 lines. Since this file is being added fresh in this PR, these references should reflect the final state of the spec file. Consider updating the line numbers to match the current positions so the references remain useful navigation aids.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/api-gaps/bubble-ups-surface.md, line 8:
<comment>Two of the three `smithy_refs` line numbers in `bubble-ups-surface.md`'s frontmatter are inaccurate — `limit_bubble_ups` is off by 14 lines and `bubble_ups_count`/`scheduled_bubble_ups_count` is off by 7–13 lines. Since this file is being added fresh in this PR, these references should reflect the final state of the spec file. Consider updating the line numbers to match the current positions so the references remain useful navigation aids.</comment>
<file context>
@@ -0,0 +1,101 @@
+sdk_demand: high
+bc3_pr: 11628
+smithy_refs:
+ - "GetMyNotificationsOutput.bubble_ups_count/scheduled_bubble_ups_count (spec/basecamp.smithy:8577)"
+ - "GetMyNotificationsInput.limit_bubble_ups (spec/basecamp.smithy:8560)"
+ - "GetBubbleUps (spec/basecamp.smithy:8642)"
</file context>
99171db to
2643d82
Compare
doc/api/sections/my_notifications.md (codified by bc3 #11628) documents the BC5 bubble-up surface that memories-emptied-regression.md explicitly defers to a separate additive PR. Absorb it: - GetMyNotificationsOutput: add required bubble_ups_count and scheduled_bubble_ups_count (top-level UI counts, present independent of the limit_bubble_ups cap and even when the scheduled_bubble_ups array is omitted). - GetMyNotificationsInput: add optional limit_bubble_ups boolean @httpquery (true caps bubble_ups at 2 current items and omits scheduled_bubble_ups). - New GetBubbleUps operation (GET /my/readings/bubble_ups.json): a paginated bare array (50/page, Link-header pagination, page query param) of current bubble-ups first (most recently bubbled up) then scheduled bubble-ups (scheduled time). Same Notification item shape as GetMyNotifications. Added to the service operations list and tagged MyNotifications. Go wrapper: MyNotificationsService.BubbleUps follows the Link header across all pages by default; a positive page returns only that page. Get gained a non-breaking WithLimitBubbleUps() functional option. Tests: a multi-page Link-following pagination test (with X-Total-Count metadata and current-then-scheduled ordering), a single-page test, and a test proving limit_bubble_ups=true sends the param and decodes a response omitting scheduled_bubble_ups while keeping the counts. The grouped bubble_ups/scheduled_bubble_ups arrays and their item fields were already modeled and are not re-added. memories-emptied-regression.md is left intact; a new bubble-ups-surface.md entry records this absorption. Updated the GetMyNotifications conformance validator fixtures for the now-required counts and added a GetBubbleUps canary entry (validates statically; live dormant).
277533d to
afebd93
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afebd93cb2
ℹ️ 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".
| describe("bubbleUps", () => { | ||
| it("should return current and scheduled bubble-ups", async () => { |
There was a problem hiding this comment.
When GetBubbleUps returns an error such as 401, 403, 429, or 500, none of the new TypeScript, Ruby, or Python tests verifies the SDK's error translation. The fresh test files added in this revision address only the happy path, so the earlier completeness issue remains partially unresolved; add an error-case test in each SDK as required for every new operation.
AGENTS.md reference: AGENTS.md:L299-L308
Useful? React with 👍 / 👎.
Closes #425. Part of the post-#401 BC3 follow-up absorption program (PR-3).
Summary
doc/api/sections/my_notifications.md(codified by bc3 #11628) documents the BC5 bubble-up surface thatmemories-emptied-regression.mdexplicitly defers to a separate additive PR. This absorbs it.Changes
GetMyNotificationsOutput: requiredbubble_ups_count+scheduled_bubble_ups_count.GetMyNotificationsInput: optionallimit_bubble_upsboolean@httpQuery(truecapsbubble_upsat 2 and omitsscheduled_bubble_ups).GetBubbleUpsop (GET /my/readings/bubble_ups.json): paginated bare array (50/page, Link-header pagination,pageparam), current bubble-ups first then scheduled. Added to the service list + tagged.MyNotificationsService.BubbleUps(Link-follows all pages by default; positivepage= single page);Getgained a non-breakingWithLimitBubbleUps()option.Tests
Go: an explicit multi-page Link-following pagination test (with
X-Total-Countmetadata + current-then-scheduled ordering), a single-page test, and a test provinglimit_bubble_ups=truepermits omission ofscheduled_bubble_upswhile keeping the counts.Registry & notes
New
bubble-ups-surface.mdentry (absorbed-in-sdk);memories-emptied-regression.mdleft fully intact (its scope is the subtractive empty-memoriesdelta and itspairwise-retirement explanation). Updated theGetMyNotificationsconformance validator fixtures for the now-required counts; added aGetBubbleUpscanary entry (validates statically; live dormant).Verification
make generate && make check— clean (24 api-gap entries validated).Summary by cubic
Adds the Bubble Ups successor surface: required counts in
GetMyNotifications, alimit_bubble_upsquery, and a dedicated, paginatedGET /my/readings/bubble_ups.json. This reduces over-fetching and lets clients page through current and scheduled bubble-ups.New Features
GetMyNotifications: requiredbubble_ups_countandscheduled_bubble_ups_count; optionallimit_bubble_upscapsbubble_upsat 2 and omitsscheduled_bubble_ups.GET /my/readings/bubble_ups.json: paginated bare array (50/page, Link pagination,X-Total-Count), current first then scheduled; retries on 429/503.MyNotificationsService.BubbleUps()andWithLimitBubbleUps(); TypeScript/Python/Ruby/Swift/Kotlin add matching methods/options and decode tests (Kotlin addsNotification/PreviewableAttachmentmodels).GetBubbleUpsadded.Migration
bubble_ups_countandscheduled_bubble_ups_countinGetMyNotificationsresponses.limit_bubble_upsfor lightweight badge fetches.GET /my/readings/bubble_ups.jsonto page the full list (50/page; current then scheduled).Written for commit afebd93. Summary will update on new commits.